Skip to content
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

fix(Icons): prevent icons from having same IDs (duplicate-id violation) #1714

Merged
merged 3 commits into from
Nov 10, 2022

Conversation

tujoworker
Copy link
Member

Thanks for @dinarosv for reporting 🙏

After the change, SVG IDs get the file-name, so they will be unique per icon. But not per page. So that's still an issue. And can lower a lighthouse score, still.

IDs in SVGs are used in order to link the usage of clipPath.

Its the Figma export that sets this. 52 icons are effected (from a total of 260).

E.g. the thumb_up (16px) uses this, but not thumb_up (24px) – I'm not sure why this difference. We may try to find out more about it.

Approaches we can do to enhance this beyond this PR:

  • find out more about, why Figma uses clipPath on some icons, and sddenly not on very similar icons.
  • generate an unique ID during React render. Not streight forward to make it work.
  • remove all usage of clipPath. What are the side-effects? They can be different from icon to icon.
  • try to find a way to transform the clipPath usage. But that's not so easy. There is an open issue/request for it.

In this PR, we get this change:

Screenshot 2022-11-10 at 08 53 31

Screenshot 2022-11-10 at 08 52 43

@tujoworker tujoworker requested review from langz and dinarosv November 10, 2022 08:40
@codesandbox-ci
Copy link

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit a4a2791:

Sandbox Source
eufemia-starter Configuration

Copy link
Contributor

@dinarosv dinarosv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the super quick fix 🙌

@gatsby-cloud
Copy link

gatsby-cloud bot commented Nov 10, 2022

✅ DNB Eufemia Portal deploy preview ready

Copy link
Contributor

@langz langz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tujoworker tujoworker merged commit 5e4079d into main Nov 10, 2022
@tujoworker tujoworker deleted the fix/icon-ids branch November 10, 2022 12:16
tujoworker pushed a commit that referenced this pull request Nov 17, 2022
# [9.38.0-beta.1](v9.37.0...v9.38.0-beta.1) (2022-11-17)

### Bug Fixes

* add support to IS_SAFARI_DESKTOP for Safari v16 on macOS ([#1718](#1718)) ([54e2cba](54e2cba))
* **Anchor:** export types as AnchorAllProps and original instance ([#1715](#1715)) ([92ec784](92ec784))
* **Icons:** prevent icons from having same IDs (duplicate-id violation) ([#1714](#1714)) ([5e4079d](5e4079d))
* **Provider:** rewrite to functional component ([#1731](#1731)) ([b504d06](b504d06))
* **Table:** align odd/even modifiers with CSS nth ([#1724](#1724)) ([8bdad07](8bdad07))

### Features

* **Table:** add "fixed" prop for fixed table layouts ([#1708](#1708)) ([241ee0f](241ee0f))
* **Table:** add Table.SortButton ([#1709](#1709)) ([288a8db](288a8db))
* **Table:** add Th.HelpButton to be used in Table Headers ([#1711](#1711)) ([c142323](c142323))
* **Th:** add table header sortable props ([#1706](#1706)) ([c40393a](c40393a))
* **Tr:** automate odd/even and make it overridable ([#1705](#1705)) ([d73d3cb](d73d3cb))
* **Upload:** support files dropped on the document body ([#1719](#1719)) ([f206243](f206243))
@tujoworker
Copy link
Member Author

🎉 This PR is included in version 9.38.0-beta.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

tujoworker pushed a commit that referenced this pull request Nov 22, 2022
# [9.38.0](v9.37.0...v9.38.0) (2022-11-22)

### Bug Fixes

* add support to IS_SAFARI_DESKTOP for Safari v16 on macOS ([#1718](#1718)) ([54e2cba](54e2cba))
* **Anchor:** export types as AnchorAllProps and original instance ([#1715](#1715)) ([92ec784](92ec784))
* **Icons:** prevent icons from having same IDs (duplicate-id violation) ([#1714](#1714)) ([5e4079d](5e4079d))
* **Provider:** rewrite to functional component ([#1731](#1731)) ([b504d06](b504d06))
* **Table:** align odd/even modifiers with CSS nth ([#1724](#1724)) ([8bdad07](8bdad07))

### Features

* **Table:** add "fixed" prop for fixed table layouts ([#1708](#1708)) ([241ee0f](241ee0f))
* **Table:** add table "border" and "outline" property ([#1739](#1739)) ([ad63ffb](ad63ffb))
* **Table:** add Table.ScrolView to support horizontal scroll ([#1735](#1735)) ([85a4d86](85a4d86))
* **Table:** add Table.SortButton ([#1709](#1709)) ([288a8db](288a8db))
* **Table:** add TableContainer to stack tables with an outline ([#1740](#1740)) ([376ac06](376ac06))
* **Table:** add Th.HelpButton to be used in Table Headers ([#1711](#1711)) ([c142323](c142323))
* **Table:** support rowSpan ([#1733](#1733)) ([463692d](463692d))
* **Th:** add table header sortable props ([#1706](#1706)) ([c40393a](c40393a))
* **Tr:** automate odd/even and make it overridable ([#1705](#1705)) ([d73d3cb](d73d3cb))
* **Typography:** support styles for superscript and subscript elements ([#1721](#1721)) ([c2b043d](c2b043d))
* **Upload:** support files dropped on the document body ([#1719](#1719)) ([f206243](f206243))
@tujoworker
Copy link
Member Author

🎉 This PR is included in version 9.38.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@buzinas
Copy link

buzinas commented Feb 17, 2023

Just so y'all know, Figma exports clipPath when "Clip content" is checked:

image

In the majority of cases, you don't need to clip any content. So if you uncheck it, there are no visual changes, but the exported SVG doesn't have this clipPath garbage:

Before After
<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" viewBox="0 0 16 16" fill="currentColor"><defs><path id="export-a" d="M12.4545455,14.125 L13.7272727,14.125 L13.7272727,13.5 L12.4545455,13.5 L12.4545455,14.125 Z M15,16 L1,16 L1,12.875 C1,12.53125 1.28636364,12.25 1.63636364,12.25 L14.3636364,12.25 C14.7136364,12.25 15,12.53125 15,12.875 L15,16 Z M6.09090909,1.625 C6.09090909,1.28125 6.37727273,1 6.72727273,1 L9.27272727,1 C9.62272727,1 9.90909091,1.28125 9.90909091,1.625 L9.90909091,6 L13.0209091,6 C13.3709091,6 13.4663636,6.20875 13.2321818,6.465 L8.36018182,11.785 C8.126,12.04125 7.74418182,12.04 7.51127273,11.7825 L2.69527273,6.4675 C2.46363636,6.21 2.55909091,6 2.90909091,6 L6.09090909,6 L6.09090909,1.625 Z"/></defs><use fill-rule="evenodd" xlink:href="#export-a"/></svg> <svg viewBox="0 0 16 16" fill="none" xmlns="http://www.w3.org/2000/svg"><path d="M12.4545 14.125H13.7273V13.5H12.4545V14.125ZM15 16H1V12.875C1 12.5312 1.28636 12.25 1.63636 12.25H14.3636C14.7136 12.25 15 12.5312 15 12.875V16ZM6.09091 1.625C6.09091 1.28125 6.37727 1 6.72727 1H9.27273C9.62273 1 9.90909 1.28125 9.90909 1.625V6H13.0209C13.3709 6 13.4664 6.20875 13.2322 6.465L8.36018 11.785C8.126 12.0412 7.74418 12.04 7.51127 11.7825L2.69527 6.4675C2.46364 6.21 2.55909 6 2.90909 6H6.09091V1.625Z" fill="currentColor" /></svg>

cc @tujoworker @dinarosv @langz

joakbjerk pushed a commit that referenced this pull request Mar 27, 2023
…n) (#1714)

* fix(Icons): prevent icons from having same IDs (duplicate-id violation)

* Refetch icons with the command: `yarn figma:reset`

* Update snapshots with new order

Because we removed the original timestamp with the reset.
@ivanbanov
Copy link

thanks for pointing out @buzinas ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

5 participants