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.

Sign up for GitHub

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

Jump to bottom

feat(icons): add new icons and reformat #71

Merged
merged 1 commit into from
Dec 12, 2023
Merged

feat(icons): add new icons and reformat #71

merged 1 commit into from
Dec 12, 2023

Conversation

mnlfischer
Copy link
Contributor

Description

Add 3 new icons and use newest svgr package to reformat all icons.

Checklist

  • Change increases quality
  • Change is validated by tests
  • Change is readable and easy to understand
  • Documentation is updated
  • Security impact has been considered
  • Changes validated in runtime

@linear Linear
Copy link

linear bot commented Dec 11, 2023

PLA-889 Add new navigation icons to Hailstorm

@mnlfischer mnlfischer requested review from a team and rodelta and removed request for a team December 12, 2023 09:27
@pallendes
Copy link
Contributor

Why are we leaving those three icons in the assets/ folder? Wouldn't it be better to translate them to SVGr and leave them with the rest?

@mnlfischer
Copy link
Contributor Author

Why are we leaving those three icons in the assets/ folder? Wouldn't it be better to translate them to SVGr and leave them with the rest?

Not sure what you ask for but the asset is our source of truth from Figma. Svgr translate all icons inside the assets folder, optimize code and create the react component for us. I think it is easer to drop the svg into assets and run the build:icon process without touching any file. If we want to modify something I would usually modify the svg because it is the unprocessed icon and can be reimported into Figma and in addition I would not trust the icons inside the Evergreen lib. Does it answer your question?

@pallendes
Copy link
Contributor

pallendes commented Dec 12, 2023 •
edited
Loading

Why are we leaving those three icons in the assets/ folder? Wouldn't it be better to translate them to SVGr and leave them with the rest?

Not sure what you ask for but the asset is our source of truth from Figma. Svgr translate all icons inside the assets folder, optimize code and create the react component for us. I think it is easer to drop the svg into assets and run the build:icon process without touching any file. If we want to modify something I would usually modify the svg because it is the unprocessed icon and can be reimported into Figma and in addition I would not trust the icons inside the Evergreen lib. Does it answer your question?

Sorry for the confusion; I thought you were creating a new assets folder to put SVG icons inside and that we would have both react SVG components and SVG files. Now I see that all the svg icons are there, and svgr is used to translate them, so it is all good 👍

@mnlfischer mnlfischer merged commit 7fa97be into main Dec 12, 2023
6 checks passed
@mnlfischer mnlfischer deleted the pla-889-add-new-navigation-icons-to-hailstorm branch December 12, 2023 13:25
@abusix-bot
Copy link
Collaborator

🎉 This PR is included in version 3.5.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

@pallendes pallendes pallendes approved these changes

@rodelta rodelta Awaiting requested review from rodelta rodelta was automatically assigned from abusix/hailstorm-reviewer

Assignees

@mnlfischer mnlfischer

Labels
released
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mnlfischer @pallendes @abusix-bot