-
Notifications
You must be signed in to change notification settings - Fork 27
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
adjust icon sizing, add icon-wrapper class, closes #763 #781
Conversation
src/icons.css
Outdated
* <p class='txt-xl'>Ring the <span class='icon-wrapper'><svg class='icon icon--l'><use xlink:href='#icon-bell'/></svg></span></p> | ||
* <p class='txt-s'>Keep your <span class='icon-wrapper'><svg class='icon icon--s'><use xlink:href='#icon-home'/></svg></span> safe from <span class='icon-wrapper'><svg class='icon icon--s'><use xlink:href='#icon-bug'/></svg></span></p> | ||
**/ | ||
.icon-wrapper { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just curious, what makes the !important
cut vs what doesn't?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i don't have a good answer for you. The importanted properties are the ones they definitely shouldn't ever get overriden when using this class. But the same could be said for almost every property here. Should I important everything?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would think everything except for the top
property? I guess this wrapper should do what it says on the tin so everything could be important'ized
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll important all but top and I'm going to rename the class .icon-inline-wrapper
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like a good option!
vertical-align: top !important; | ||
align-items: center !important; | ||
justify-content: center !important; | ||
height: 1em !important; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@samanpwbb: I think that conventionally we haven't been using !important
unless the declaration corresponds directly to the class name. Is there a reason we need to use !important
on these declarations?
This PR adds two new classes for centering icons inline, plus a new catalog entry for icons. The solution uses a wrapping element, which has some advantages:
The one big disadvantage being that it requires more markup. I don't think there's any other option though (trust me, I tried a lot of things here!)
The PR also increases the size of the small icon. 12px was just way too tiny. Nothing was legible.