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

Fix styling issues in Safari #1114

Merged
merged 2 commits into from
Dec 7, 2016
Merged

Fix styling issues in Safari #1114

merged 2 commits into from
Dec 7, 2016

Conversation

ebidel
Copy link
Contributor

@ebidel ebidel commented Dec 6, 2016

R: @paulirish @patrickhulce @brendankenny

This changes all the rating classes to have a single - prefix.

--poor is not a valid CSS class. See apache/incubator-pagespeed-ngx#993.

Safari before:

screen shot 2016-12-06 at 3 12 49 pm

After:

screen shot 2016-12-06 at 3 12 32 pm

@patrickhulce
Copy link
Collaborator

haven't really used BEM in practice, why isn't it concatenated? like .block__score--poor

@brendankenny
Copy link
Member

it'll cause more verbose css (we need two classes on there for base styling for section-result__score and section-result__points), but it does feel like if we're going to do BEM we shouldn't do mostly BEM by having only one hyphen...

@patrickhulce
Copy link
Collaborator

seems like a broken naming convention if it isn't even valid CSS...

@brendankenny
Copy link
Member

I believe the "proper" BEM thing is to have them concatenated, somebody just did .section-result__score.--good as a shortcut for something like .section-result__score.section-result__score--good

@brendankenny
Copy link
Member

IANABEOECU (I Am Not A BEM Expert Or Even Casual User)

@ebidel
Copy link
Contributor Author

ebidel commented Dec 6, 2016

This is a prime example of why I dislike BEM. Generic .poor/.good/.average classes that apply everywhere would DRY up our CSS (9 occurrences of duplicate styling).

What do you guys think about going that route?

@brendankenny
Copy link
Member

sounds perfect to me, but, again, you could tell me that was valid BEM and I would believe you, so I may not be the best person to ask :)

@brendankenny
Copy link
Member

I guess there is still validity in going all in on BEM or abandoning it completely.

@paulirish cause he has actual opinions on CSS and stuff

@ebidel
Copy link
Contributor Author

ebidel commented Dec 6, 2016

BEM is a convention, but that doesn't mean you have to BEM all the things. There are valid places to use the cascading an reusability features of CSS 💯

@patrickhulce
Copy link
Collaborator

patrickhulce commented Dec 6, 2016

This is a prime example of why I dislike BEM. Generic .poor/.good/.average classes that apply everywhere would DRY up our CSS (9 occurrences of duplicate styling). What do you guys think about going that route?

+1
For my own edification would the "BEM approach" to reuse be something like applying a block level poor/good/average mod rule? Or is there no DRY way to achieve this with BEM convention?

@paulirish
Copy link
Member

me too on IANABEOECU. :)

i'm fine with whatever in the CSS, tbh. It doesn't stress me out.

@ebidel
Copy link
Contributor Author

ebidel commented Dec 7, 2016

PTAL

Copy link
Member

@brendankenny brendankenny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🎨

@brendankenny brendankenny merged commit 9b69ee5 into master Dec 7, 2016
@brendankenny brendankenny deleted the safaristyles branch December 7, 2016 01:18
andrewrota pushed a commit to andrewrota/lighthouse that referenced this pull request Jan 13, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants