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
davidrapson
force-pushed
the
toggle-svg-icons
branch
from
98c3c28 to
cd3eb94
Compare
davidrapson
removed
the
docs preview
There was a problem hiding this comment.
Nice to be closer to ditching the icon font 🙌🏻
We use the abstracted show/hide quite a lot in content platform. I think will cause a lot of code duplication in that codebase, but we can abstract it out there instead if we need to.
I'm in favour of not abstracting stuff unless it's useful - I think this is useful but it's not a strongly held opinion. Was changing to an icon font difficult because of the shared show/hide code, or has it made something else harder to change somewhere?
|
Moving this back to draft as it it needs updating and I'd like to resolve the outstanding question on the show/hide helper (My stance is that we should ditch it in the long term as it still relies on the icon font and is too generic/internal for consumers to be relying on but we should reinstate the helper in this PR so it doesn't need to block this going in) |
davidrapson
force-pushed
the
toggle-svg-icons
branch
from
cd3eb94 to
38a6005
Compare
|
Update this to bring it up to date with Refreshed my memory a bit on where we left things on the show-hide utility and checked a couple more things: The existing utility ultimately works based on the old icon font which a) we want to remove and b) works very differently to inline SVG icons. I’m not entirely sure what a generic utility for this would look like for those 🤔 I’d wondered if public-website was relying on this but it turns out it has it’s own copy of show-hide and doesn’t load the one here anyway which makes things simpler (maybe that came first?) so I think we’re safe to remove it as nothing is using and it would need a significant re-think to be useful with the SVG icon setup. We should see if a suitable alternative comes out of removing icon font uses from public website if it turns out there is a revised version of this utility that works. Does that sound good to folks? |
davidrapson
force-pushed
the
toggle-svg-icons
branch
from
38a6005 to
dc40ae3
Compare
davidrapson
force-pushed
the
toggle-svg-icons
branch
from
9347fab to
7d04c13
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
7d04c13 to
f2ff212
Compare
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.