Update on this page and disclosure components to use SVG icons #3261
Conversation
davidrapson
force-pushed
the
toggle-svg-icons
branch
5 times, most recently
from
0a30cef
to
24e0efe
Compare
| it { is_expected.to have_css "[data-toggle-target-id='my-id']" } | ||
| it { is_expected.to have_css "[aria-controls='my-id']" } |
There was a problem hiding this comment.
This has been switched out as the component no longer needs data-toggle-target-id to work. Testing against aria attributes is preferable too.
davidrapson
requested review from
leeky,
akamike,
cnorthwood,
marianayovcheva and
davidsauntson
and removed request for
leeky and
akamike
marianayovcheva
added
the
docs preview
...ata/bitmaps_reference/backstop_default_Components_Disclosure_fallback_0_document_0_small.png
Outdated
Show resolved
Hide resolved
|
Yay for moving away from the icon font! 🎉 Looks really good! |
My (loosely held) but strong preference is to keep them separate, especially as we only have two examples of slightly similar code. Not against extracting it again in future though. I'm very much optimising for "make the components easy to change" vs "share the most code" here. |
davidrapson
force-pushed
the
toggle-svg-icons
branch
from
24e0efe
to
98c3c28
Compare
This change removes the use of icon fonts from the disclosure component. To enable this the component has been refactored to inline the toggle logic rather than sharing it between two components to make per-component changes simpler to manage. This change also makes sure the component consistently uses cads prefixed classes including for js-cads classes.
This change removes the use of icon fonts from the on this page component. To enable this the component has been refactored to inline the toggle logic rather than sharing it between two components to make per-component changes simpler to manage. Includes an additional cypress tests to make sure we have test coverage as some is lost by inlining the shared code.
This behaviour is now included and tested on a per-component basis.
davidrapson
force-pushed
the
toggle-svg-icons
branch
from
98c3c28
to
cd3eb94
Compare
davidrapson
removed
the
docs preview
Background
We currently ship two different sets of icons within the design system. A new SVG icon set and the previous icon font. We've long wanted to remove the icon font completely but to do that we need to ensure that no internal components are using icon fonts.
This pull request makes steps to remove them from two related components: On this page and disclosure.
Changes
Inline toggle code
This is the biggest surface change. The on this page and disclosure components previously shared code via a utility method. In retrospect this makes it quite hard to change each component independently and in practice the disclosure component had subtly different behaviour which had to be patched in separately.
Practically these are both an implementation of https://www.w3.org/WAI/ARIA/apg/patterns/disclosure/, much like targeted content is so it's reasonable to duplicate the pattern here to keep things flexible.
To account for this an additional cypress test has been added to cover the on this page toggle behaviour. This is preferable to the previous test anyway as it checks against a real component example rather than the lower level fixture used by the utility code.
Use SVG icons in place of icon fonts
For both components the same pattern has been used to make use of the
PlusMinusicon component. Like with previous versions of this move the SVG icons are slightly thinner than the icon versions so there is a small visual change.