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

Select - refactor icons #450

Merged
merged 6 commits into from
Aug 16, 2024
Merged

Select - refactor icons #450

merged 6 commits into from
Aug 16, 2024

Conversation

mkernohanbc
Copy link
Contributor

This change updates the Select component to use componentised versions of icons, rather than instantiating icons as functions within the component itself.

  • fac327c replaces iconError with the SvgExclamationIcon component, and adds some additional CSS to style it
  • c4ecae4 adds two new icon components (SvgChevronUpIcon and SvgChevronDownIcon)
  • ca25172 replaces ChevronUp and ChevronDown functions with the new components

@mkernohanbc mkernohanbc added the enhancement New feature or request label Aug 13, 2024
@mkernohanbc mkernohanbc added this to the Components v0.2.0 milestone Aug 13, 2024
@mkernohanbc mkernohanbc self-assigned this Aug 13, 2024
@mkernohanbc mkernohanbc linked an issue Aug 13, 2024 that may be closed by this pull request
@mkernohanbc mkernohanbc requested a review from ty2k August 13, 2024 23:51
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ty2k do we need the React import on line 1, or is that legacy stuff?

Copy link
Contributor

Choose a reason for hiding this comment

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

We do need it in this case, because we're using it for our TypeScript definitions, like iconLeft?: React.ReactElement;.

@ty2k
Copy link
Contributor

ty2k commented Aug 16, 2024

Couple things with this one:

In our component files, we don't want be to using the Vite resolve.alias @ path when we're importing other files. This causes circular dependency warnings when we run the Rollup script that builds the component library for publishing. In general, we don't want to introduce new warnings in this process's log to keep it useful for us. The existing warnings we have in this output are from our node_modules dependencies which we don't have direct control over. Below is an example where the InlineAlert is causing a warning. I will push a commit to this branch that fixes this in both the InlineAlert and Select.

Terminal output of Rollup script showing warning for circular dependency on InlineAlert

Secondly, we should avoid extraneous <span> elements around the SVGs where possible. In this case we are able to avoid it. I will push a commit to this branch that fixes this.

@ty2k
Copy link
Contributor

ty2k commented Aug 16, 2024

Select before and after (no extra spacing on the exclamation icon by avoiding the <span>)

Before
Select before this PR
Select after this PR

InlineAlert before and after (no changes):
InlineAlert before this PR
InlineAlert after this PR

@ty2k ty2k merged commit bb061f8 into main Aug 16, 2024
4 checks passed
@ty2k ty2k deleted the 441-select-refactor-icons branch August 16, 2024 16:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Select - refactor icons
2 participants