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

feat(Icons): allow component icon usages to be headless #1761

Merged
merged 1 commit into from
Sep 29, 2023

Conversation

booc0mtaco
Copy link
Contributor

@booc0mtaco booc0mtaco commented Sep 20, 2023

Summary:

  • part one: using existing icons with current selection as default
    • this will let teams choose from the possible set of icons when overrides are needed
    • some cases only allow one icon for now but abstract this for future use
    • rename any icon prop names to icon for consistency and backwards compatibility
  • part two will enable using a ReactNode or some render prop, so teams can use custom icons with their own types

Test Plan:

  • Wrote automated tests
  • CI tests / new tests are not applicable
  • Manually tested my changes, but I want to keep the details secret
  • Manually tested my changes, and here are the details:
    • Create an alpha publish and try out in edu-stack or traject 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.

@codecov
Copy link

codecov bot commented Sep 20, 2023

Codecov Report

Merging #1761 (380ce7d) into next (564e8af) will increase coverage by 0.05%.
The diff coverage is 100.00%.

@@            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              
Files Coverage Δ
src/components/Accordion/Accordion.tsx 96.55% <100.00%> (+0.06%) ⬆️
src/components/Avatar/Avatar.tsx 100.00% <100.00%> (ø)
src/components/Badge/Badge.tsx 100.00% <100.00%> (ø)
src/components/Breadcrumbs/Breadcrumbs.tsx 95.52% <100.00%> (+0.06%) ⬆️
src/components/DragDrop/DragDrop.tsx 67.88% <100.00%> (+0.47%) ⬆️
src/components/FieldNote/FieldNote.tsx 100.00% <100.00%> (ø)
...components/HorizontalStepper/HorizontalStepper.tsx 100.00% <100.00%> (ø)
src/components/Icon/Icon.tsx 100.00% <ø> (ø)
...mponents/InlineNotification/InlineNotification.tsx 95.83% <100.00%> (ø)
src/components/Menu/Menu.tsx 98.00% <100.00%> (+0.08%) ⬆️
... and 8 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@github-actions
Copy link

github-actions bot commented Sep 20, 2023

size-limit report 📦

Path Size
components 95.65 KB (+0.19% 🔺)
styles 32.75 KB (-0.04% 🔽)

@booc0mtaco booc0mtaco force-pushed the aholloway/EDS-1108 branch 2 times, most recently from e6217dc to 8ce1d63 Compare September 21, 2023 00:20
/**
* Icon override for component. Default is 'expand-more'
*/
icon?: Extract<IconName, 'expand-more'>;
Copy link
Contributor Author

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)

Copy link
Contributor

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).

Copy link
Contributor Author

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);
Copy link
Contributor Author

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.

@booc0mtaco booc0mtaco force-pushed the aholloway/EDS-1108 branch 2 times, most recently from 69d0b39 to 688abe4 Compare September 22, 2023 21:33
@booc0mtaco booc0mtaco marked this pull request as ready for review September 22, 2023 21:35
@booc0mtaco booc0mtaco requested a review from a team September 22, 2023 21:38
@booc0mtaco
Copy link
Contributor Author

This also includes some storybook tweaks to make the Drag Drop component more generic.

Screenshot 2023-09-22 at 17 39 14

- 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
@booc0mtaco booc0mtaco merged commit ba454bf into next Sep 29, 2023
8 checks passed
@booc0mtaco booc0mtaco deleted the aholloway/EDS-1108 branch September 29, 2023 20:48
@booc0mtaco booc0mtaco mentioned this pull request Oct 3, 2023
booc0mtaco added a commit that referenced this pull request Oct 4, 2023
## [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))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants