-
Notifications
You must be signed in to change notification settings - Fork 7
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
Feat/svg icons 1396 #1524
Feat/svg icons 1396 #1524
Conversation
engine/app/components/citizens_advice_components/icons/arrow_down.html.erb
Outdated
Show resolved
Hide resolved
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.
Looking great so far. Only thing I'm a little stuck on is the icon name behaviour. I think I might be missing where that's used.
And had a little question about the composition + inheritance for the base class. Wondering if the base icon class we inherit from is a different concept to the base icon component we render inside each icon.
engine/app/components/citizens_advice_components/icons/arrow_down.html.erb
Outdated
Show resolved
Hide resolved
engine/app/components/citizens_advice_components/targeted_content.html.haml
Outdated
Show resolved
Hide resolved
engine/previews/citizens_advice_components/icons_preview/icons.html.haml
Show resolved
Hide resolved
engine/app/components/citizens_advice_components/targeted_content.html.haml
Outdated
Show resolved
Hide resolved
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 great! I reckon the heading-wrapper
class in targeted content should go, or be a separate change if we do need it but otherwise fab.
Adds icon svg view components to the design system:
CitizensAdviceComponents::Icons::Base
, which renders outer svg and associated props/assets
(see below)This PR is already quite noisy so I want to complete the following tasks in separate PRs:
/assets
(not used as far as I can tell), add it into the package, test in public-website and update icon docsCitizensAdviceComponents::Button
(and anything else?)Partial fix for #1396