Fix icons/logos with static SVG IDs to use generated IDs #5204

cee-chen · 2021-09-21T15:48:15Z

Summary

⚠️ This PR is primarily whitespace/indenting changes - I recommend viewing diffs with whitespace changes disabled.

This PR fixes all icons/logos with static SVG ids (used for masks/fills/defs, etc.) to use our HTML ID generator utility, making every icon have its own unique set of IDs.

The issue with static IDs on our logo/icon SVGs is that they create semantic/accessibility errors when you use multiples of those icons on the same page, when only 1 unique ID should exist per-page.

Before

After:

(NB: all other issues on the page are pre-existing, we're only looking at the minor count for ID issues)

QA

Go to https://eui.elastic.co/pr_5204/#/display/icons#elastic-logos and confirm that the testing logos have unique IDs, and that running axe-devtools does not produce any minor errors about id attribute value must be unique

Checklist

Check against all themes for compatibility in both light and dark modes

- [ ] Checked in mobile

Checked in Chrome, Safari, Edge, and Firefox

- [ ] Props have proper autodocs and playground toggles
- [ ] Added documentation
~~- [ ] Checked Code Sandbox works for any docs examples~~

Added or updated jest tests

~~- [ ] Checked for breaking changes and labeled appropriately~~

Checked for accessibility including keyboard-only and screenreader modes
A changelog entry exists and is marked appropriately
Revert 49b228e

cee-chen · 2021-09-21T16:09:11Z

src/components/icon/assets/logo_apache.js

+import { htmlIdGenerator } from '../../../services';
+
+const EuiIconLogoApache = ({ title, titleId, ...props }) => {
+  const generateId = htmlIdGenerator('logo_apache');


You might ask: "Constance, you just created a useGeneratedHtmlId hook for functional components, why not use that here?"

To which I would say: Great question! The answer is because useGeneratedHtmlId() generates a completely random UUID each time, and I actually didn't want that for these logo IDs - I wanted their UUIDS to be the shared between each SVG/icon instance, and simply use different suffixes, like so:

This is also a good illustration/use-case of when to use the use hook (simple/singleton IDs) vs the original generator utility.

I'll also add I'm not concerned about the IDs regenerating for these icons: they have no internal state and won't rerender from that, so they'll only update when they receive new props dynamically, which I anticipate being fairly rare (and usage of these specific logos are rare/possibly will be deprecated, and and seem to be limited to Kibana currently in any case).

kibanamachine · 2021-09-21T16:09:19Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5204/

thompsongl

Hey we'll need to find a more general approach to solving this problem: these files are generated and the changes here will be undone the next time yarn compile-icons run 🤖. Output below:

I think all changes need to be scoped to https://github.com/elastic/eui/blob/b23c9d7020526df7b5f971e2cae32e9a9246f9c5/scripts/compile-icons.js or to the original .svg files.

thompsongl · 2021-09-22T14:02:20Z

Relatedly, we should add a THIS IS A GENERATED FILE. DO NOT MODIFY MANUALLY. comment to the template in compile-icons.js so this is more clear.

cee-chen · 2021-09-22T15:18:21Z

Oh snap! I hadn't realized that, sorry. Will take a look at that generated comment as well!

cee-chen · 2021-09-22T17:50:23Z

Going to do the generated comment in a separate PR since it touches a whole lot more icon files than I want to have in the diff for this specific PR/issue: #5210

- DRY out toString() to one var - switch from indexOf to the slightly more readable `.includes()` API

- Use different template with htmlIdGenerator import/setup + non-implicit return - tweak template literal indenting - DRY out fileName for use as generator prefix

NOTE: I'm avoiding doing this in the `ast` process for two main reasons: 1. I'm a monster who think regexes are neat 2. the ${jsx} in the ast template is an actual tree of components and it seems much more tedious to iterate through each one & examine its attributes (of which the url() attr can have multiple - e.g. fill or mask), rather than use a basic regex capture group

- in their current state, all icons (except for dropwizard) will automatically truncate their ID names to 'a', 'b', etc. when cleanupIDs is set to true

moment of truth!

cee-chen · 2021-09-22T20:59:53Z

@thompsongl Alrighty, compile-icons.js has been updated to automatically output the same changes I made manually! 🎉

Let me know what you think (and also if you think I'm a monster for using regexes)!

kibanamachine · 2021-09-22T21:24:23Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5204/

thompsongl

This is great! Just a couple things before merging:

Revert 49b228e
Merge/rebase master and update the changelog entry location

(Also, you're not a regex monster 👹)

This reverts commit 49b228e.

cee-chen · 2021-09-23T16:27:03Z

Master merged, changelog updated, revert made, and auto-merge enabled 🚀

kibanamachine · 2021-09-23T16:51:15Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5204/

cee-chen added the accessibility label Sep 21, 2021

cee-chen force-pushed the logo-generated-ids branch from 22a233e to 72e2c68 Compare September 21, 2021 15:50

cee-chen changed the title ~~Logo generated ids~~ Fix icons/logos with static SVG IDs to use generated IDs Sep 21, 2021

cee-chen commented Sep 21, 2021

View reviewed changes

cee-chen marked this pull request as ready for review September 21, 2021 16:09

thompsongl reviewed Sep 22, 2021

View reviewed changes

cee-chen mentioned this pull request Sep 22, 2021

[EuiIcon] Add generated file comment #5210

Merged

cee-chen added 3 commits September 22, 2021 12:56

Remove unnecessary ID from stopSlash icon

724bbd2

[scripts setup] Add check for static SVG IDs

8ce769b

- DRY out toString() to one var - switch from indexOf to the slightly more readable `.includes()` API

Set up htmlIdGenerator util for SVGs with IDs

b6745e4

- Use different template with htmlIdGenerator import/setup + non-implicit return - tweak template literal indenting - DRY out fileName for use as generator prefix

cee-chen force-pushed the logo-generated-ids branch from 72e2c68 to f0488ac Compare September 22, 2021 20:52

cee-chen added 6 commits September 22, 2021 13:56

Clean up ID prefixes

e3b0582

- in their current state, all icons (except for dropwizard) will automatically truncate their ID names to 'a', 'b', etc. when cleanupIDs is set to true

Run yarn compile-icons

0de9359

moment of truth!

Update snapshots

306c375

Add changelog entry

d435936

[REVERT ME] Documentation example to test IDs/changes

49b228e

cee-chen force-pushed the logo-generated-ids branch from f0488ac to 49b228e Compare September 22, 2021 20:57

cee-chen requested a review from thompsongl September 22, 2021 20:59

thompsongl approved these changes Sep 23, 2021

View reviewed changes

cee-chen added 3 commits September 23, 2021 09:23

Merge branch 'master' into logo-generated-ids

443efdd

Update changelog

9942426

Revert "[REVERT ME] Documentation example to test IDs/changes"

19249a4

This reverts commit 49b228e.

cee-chen enabled auto-merge (squash) September 23, 2021 16:27

cee-chen merged commit 0da641c into elastic:master Sep 23, 2021

cee-chen deleted the logo-generated-ids branch September 23, 2021 16:59

breehall mentioned this pull request Oct 1, 2021

Bumping EUI to 39.0.0 elastic/kibana#113633

Merged

cee-chen mentioned this pull request Oct 25, 2021

(Accessibility) Tutorial_Directory - Multiple elements on the page use the same id elastic/kibana#89493

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix icons/logos with static SVG IDs to use generated IDs #5204

Fix icons/logos with static SVG IDs to use generated IDs #5204

cee-chen commented Sep 21, 2021 •

edited by thompsongl

Loading

cee-chen Sep 21, 2021 •

edited

Loading

kibanamachine commented Sep 21, 2021

thompsongl left a comment •

edited

Loading

thompsongl commented Sep 22, 2021

cee-chen commented Sep 22, 2021

cee-chen commented Sep 22, 2021

cee-chen commented Sep 22, 2021

kibanamachine commented Sep 22, 2021

thompsongl left a comment

cee-chen commented Sep 23, 2021

kibanamachine commented Sep 23, 2021

Fix icons/logos with static SVG IDs to use generated IDs #5204

Fix icons/logos with static SVG IDs to use generated IDs #5204

Conversation

cee-chen commented Sep 21, 2021 • edited by thompsongl Loading

Summary

Before

After:

QA

Checklist

cee-chen Sep 21, 2021 • edited Loading

Choose a reason for hiding this comment

kibanamachine commented Sep 21, 2021

thompsongl left a comment • edited Loading

Choose a reason for hiding this comment

thompsongl commented Sep 22, 2021

cee-chen commented Sep 22, 2021

cee-chen commented Sep 22, 2021

cee-chen commented Sep 22, 2021

kibanamachine commented Sep 22, 2021

thompsongl left a comment

Choose a reason for hiding this comment

cee-chen commented Sep 23, 2021

kibanamachine commented Sep 23, 2021

cee-chen commented Sep 21, 2021 •

edited by thompsongl

Loading

cee-chen Sep 21, 2021 •

edited

Loading

thompsongl left a comment •

edited

Loading