Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Restyle elevation 5 as 12dp; add elevation 6 #34

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

keanulee
Copy link
Contributor

Required to update the raised elevation for paper-fab. See PolymerElements/paper-fab#35

Technically this is a breaking change for anyone who uses elevation="5" (it is now a 12dp shadow instead of 16dp), but AFAIK none of our elements rely on this, and this purely cosmetic change will have little impact where it is used.

@addyosmani
Copy link
Contributor

Change LGTM. Per the comment in PolymerElements/paper-fab#35 (comment) looks like we're fine adding the docs after this lands. This is a breaking change so will we want to reflect that in the versioning? 🐑

@keanulee
Copy link
Contributor Author

keanulee commented Feb 1, 2016

@addyosmani While this is technically a breaking change, I think we want to hold off on a major version bump - simply updating the styling doesn't feel like a major breaking change.

As for paper-fab, it's styling won't actually be corrected until after this PR lands and I can update its calculateElevation method. That also doesn't feel like a major change, since this is essentially "fixing" its styles.

@notwaldorf
Copy link
Contributor

Breaking the style in general, out of context, is definitely a breaking change, friends, whether it breaks our elements or anybody who uses this element. The argument can be made, however, that in this case we've just implemented the wrong height for this elevation. I tried to see how many people used elevation 5, but the GitHub text search is garbage and I got 75k results of not actually elevation="5".

The change LGTM, but pinging @cdata to keep us honest about the version.

@keanulee
Copy link
Contributor Author

ping @cdata

1 similar comment
@btelles
Copy link

btelles commented Feb 17, 2016

ping @cdata

@notwaldorf
Copy link
Contributor

@keanulee I'm going to remove myself from this so that I can clean up my incoming PR queue :)

@notwaldorf notwaldorf removed their assignment May 23, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants