-
Notifications
You must be signed in to change notification settings - Fork 4
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(Icons): allow component icon usages to be headless #1761
Conversation
Codecov Report
@@ Coverage Diff @@
## next #1761 +/- ##
==========================================
+ Coverage 92.19% 92.24% +0.05%
==========================================
Files 146 146
Lines 2588 2606 +18
Branches 666 679 +13
==========================================
+ Hits 2386 2404 +18
Misses 201 201
Partials 1 1
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
size-limit report 📦
|
e6217dc
to
8ce1d63
Compare
/** | ||
* Icon override for component. Default is 'expand-more' | ||
*/ | ||
icon?: Extract<IconName, 'expand-more'>; |
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.
This might not look all that useful but hear me out :)
Firstly, this is a first step in creating a new union utility type that replaces expand-more
with T
along with some other functionality. that will allow for injecting custom icons with some constraints. There is some architecture work to do for that final state so replacing the cases now in anticipation of that functionality. Using Extract
like this adds some ergonomics for when the the available icons change in the future, or when teams provide their own Icon sets (and thus their own IconName
potentially)
- Allow for a generic Icon component wrapper with useful constraints (control of viewport, enforce accessibility, etc.) in the type.
- Implement utility type, likely in
Icons.tsx
as a new export - Find/replace all usages of this construction with that utility type where needed (searching "Extract<IconName," makes that easy)
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.
Eventually this will allow some subset of icons?
Probably easier to allow any icon name be used (at the expense of control).
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.
This is one of the cases where it'd either be this specific built-in icon, or some custom icon that team wants to use (and not another EDS icon) when I work on the second part.
In the next week or two, hoping to have a chat with some design folks so I can start on the second part of this one, and possibly simplify parts of this further
@@ -75,16 +75,16 @@ | |||
display: flex; | |||
flex-shrink: 0; | |||
flex-direction: column; | |||
margin-left: var(--eds-size-2); | |||
margin-right: var(--eds-size-2); |
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.
Originally, it was adding margins to the front side of every drag-drop section, and a pseudo-class to chop that margin from the first thing. Flip that around so the margins get added to all but the last section, so that we don't need an override in the most common case.
For future work... this whole component will likely be moved out of EDS with all its core pieces remaining as exported atoms/molecules.
69d0b39
to
688abe4
Compare
- using existing icons with current selection as default - this will let teams choose from the possible set of icons when overrides are needed - add templated type for future expansion - update any new/existing snapshots
688abe4
to
380ce7d
Compare
## [13.4.0](v13.3.0...v13.4.0) (2023-10-03) ### Features * **Accordion:** allow empty or hidden accordion rows ([#1767](#1767)) ([e044a85](e044a85)) * **Icons:** allow component icon usages to be headless ([#1761](#1761)) ([ba454bf](ba454bf)) * **InputField:** support recommendedMaxLength prop for display-only errors ([#1771](#1771)) ([cc84a20](cc84a20)) * **Tabs:** add ability to customize tab button headers ([#1768](#1768)) ([f231ad4](f231ad4)) * **TextareaField:** support recommendedMaxLength prop for display-only errors ([#1769](#1769)) ([0852356](0852356))
Summary:
icon
for consistency and backwards compatibilityTest Plan:
edu-stack
ortraject
as a sanity check if changes affect build or deploy, or are breaking, such as token changes, widely used component updates, hooks changes, and major dependency upgrades.