Skip to content

chore: Move Seer icons into the main list #93555

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

Merged
merged 3 commits into from
Jun 16, 2025
Merged

Conversation

ryan953
Copy link
Member

@ryan953 ryan953 commented Jun 13, 2025

The icons have little animations, and look like this:

Screen.Recording.2025-06-16.at.10.23.35.AM.mov

@ryan953 ryan953 requested review from a team as code owners June 13, 2025 20:22
@github-actions github-actions bot added the Scope: Frontend Automatically applied to PRs that change frontend components label Jun 13, 2025
Copy link
Member Author

@ryan953 ryan953 Jun 13, 2025

Choose a reason for hiding this comment

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

Changed this file from the img on the left, to be the img on the right:
SCR-20250613-lviq

@@ -94,6 +94,8 @@ export {IconResize} from './iconResize';
export {IconRewind10} from './iconRewind10';
export {IconSad} from './iconSad';
export {IconSearch} from './iconSearch';
export {IconSeer} from './iconSeer';
export {IconSeerLoading, IconSeerWaiting} from './iconSeerLoading';
Copy link
Member

Choose a reason for hiding this comment

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

@roaga can we make these icons something like <IconSeer state="loading|waiting"/>, or are they so different that they warrant separate icons?

Copy link
Member

Choose a reason for hiding this comment

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

It's definitely doable that way. Not sure which approach is more semantically correct/aligns with how we want to organize icons, but I'm fine if y'all want to go with your suggestion

Copy link
Member

Choose a reason for hiding this comment

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

Having a single seer icon would be preferable then. Note that we have a couple icons that have different states (loading, circled etc).

The main difference here is in terms of imports and discoverability.

<IconSeer variant={isLoading ? 'loading' : undefined} {...props} /> 

vs

// two places where you need to worry about props.
isLoading ? <IconSeerLoading {...props}/> : <IconSeer {...props/>

A common problem with the latter is that if you use inline props, it's easy to forget to update them on all nodes (think size for example), but also adding new variants becomes more involved than you'd want, because you need to change the entire node. Having a single jsx node guides the developer to the correct outcome, which is a nice side effect of the API design.

Copy link
Member Author

Choose a reason for hiding this comment

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

i'll merge all three and add the variant: undefined | 'loading' | 'waiting' prop

Copy link
Member

@JonasBa JonasBa Jun 16, 2025

Choose a reason for hiding this comment

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

@ryan953 Thank you! I'm going to approve this and we can tackle it in a separate PR btw, so it's easier to review and we can keep this PR only about moving seer to the icon set

Copy link
Member Author

Choose a reason for hiding this comment

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

I just threw all the icon css into one file and updated the few callsites that use waiting and loading.

Also i updated the story file so each variation is rendered there... there's a video of the animations in the PR desc above.

@ryan953 ryan953 enabled auto-merge (squash) June 16, 2025 17:26
@ryan953 ryan953 merged commit d27deb8 into master Jun 16, 2025
46 checks passed
@ryan953 ryan953 deleted the ryan953/mv-seer-icons branch June 16, 2025 17:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Scope: Frontend Automatically applied to PRs that change frontend components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants