-
Notifications
You must be signed in to change notification settings - Fork 138
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
docs(icons): updates icons guidelines as part of content audit. #3689
Conversation
Issue: icon sizes at top of page are not accurate - all are the same size. Question: Is the "updated icon recommendations" section needed? It seems like it's been around for a while (?), so I'm not sure the "recently updated" wording is accurate. |
the code for changing an icon's size has changed. The best way to do it is probably just to use the icon component. To fix those icons:
We also need to update the size values on this page. "small" and "medium" changed to 12px and 16px in patternfly/patternfly#5525
I'm not sure about that table... even spot checking, I don't know if the recommendations are even current. Looks like that section and table were added 3 years ago in #1840. cc @lboehling @mmenestr do you think we need that section, or can we just remove it from the icons page? For reference, here is the page in question https://patternfly-org-pr-3689-site.surge.sh/design-foundations/icons#current-icon-recommendations |
I think having the table that outlines what icon to use when is helpful. The wording could definitely use an update though since it's not longer "recently" updated ha! |
It actually looked like the "updated" recommendations are included in the "all icons" table - so maybe the second table isn't necessary? I removed it for now, but can add back if I'm wrong. I don't know if it's worth addressing in this PR, but I find the image to be pretty wonky for mobile screen sizes. Maybe, longer term, it might be better to break out each "sm", "md", etc. visual example into their own images? I like it being in a grid and how it's formatted for desktop currently, but it doesn't seem to adapt well. |
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.
🎃👍
Looks good to me! Nice job on those code updates.
I find the image to be pretty wonky for mobile screen sizes. Maybe, longer term, it might be better to break out each "sm", "md", etc. visual example into their own images? I like it being in a grid and how it's formatted for desktop currently, but it doesn't seem to adapt well.
If you'd like a hand with any of that, just lemme know.
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.
LGTM!
Closes #3651