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

ui: Alias all our Structure Icons to Flight Icons #12209

Merged
merged 5 commits into from
Feb 2, 2022

Conversation

johncowen
Copy link
Contributor

@johncowen johncowen commented Jan 27, 2022

This PR updates our styles/base with the output from iris, so just machine generated code.

User facing change is: We are now using @hashicorp/flight-icons 🎉

Preview Link

Screenshot 2022-01-27 at 14 20 53

Small note: There are a few icons where the sizing or padding or something has changed, for example:

Screenshot 2022-01-27 at 14 19 46

I'm going to look at these an PR separately ontop of here and potentially merge those onto here, but I'd be perfectly happy for this PR to go in before those little fixups and for the whatever work is involved in fixing up those odd icons to go in after. As always, I'm easy.

cc @didoo just an FYI that we are doing this now.

@johncowen johncowen added the theme/ui Anything related to the UI label Jan 27, 2022
@johncowen johncowen requested review from jgwhite, amyrlam, a user and natmegs January 27, 2022 14:26
@johncowen
Copy link
Contributor Author

Quick note, just pushed a commit to move the aliased icons use the thicker-lined 16px icons. The change is basically this https://github.com/johncowen/iris/commit/adc72741a650183cf43770ef28ccb764d64e7f8a followed by a re-build/re-publish.

Copy link

@amyrlam amyrlam left a comment

Choose a reason for hiding this comment

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

I did some QA and found this, need to tweak the alignment. It's hard to read the code diff, I'm not sure exactly what's wrong yet... but please fix this one:

image

@amyrlam
Copy link

amyrlam commented Feb 1, 2022

Note: I have not done an exhaustive QA of the UI.

@johncowen
Copy link
Contributor Author

johncowen commented Feb 1, 2022

Thankyou! Take a look at #12222. I didn't want to merge that one down here until this was approved, this one is machine generated so wanted to keep the entire PR like that for approval. If thats the only requested change, happy to merge that (#12222) in now?

* Remove hack for the inconsistent sort icon

* Add a dark/light conditional switch and move to rem sizing for icons

* Add a bunch of new themed semantic placeholders for some icons

* Use those new placeholders where they are needed
@johncowen
Copy link
Contributor Author

K #12222 is merged, should have fixed up that stray icon, you can see the change for that in #12222

Copy link

@amyrlam amyrlam left a comment

Choose a reason for hiding this comment

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

🎨

@vercel vercel bot temporarily deployed to Preview – consul-ui-staging February 2, 2022 11:50 Inactive
@vercel vercel bot temporarily deployed to Preview – consul February 2, 2022 11:50 Inactive
@johncowen johncowen merged commit 0f94ce3 into main Feb 2, 2022
@johncowen johncowen deleted the ui/feature/flight-icons-aliasing branch February 2, 2022 13:24
@hc-github-team-consul-core
Copy link
Collaborator

🍒 If backport labels were added before merging, cherry-picking will start automatically.

To retroactively trigger a backport after merging, add backport labels and re-run https://circleci.com/gh/hashicorp/consul/572992.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr/no-metrics-test theme/ui Anything related to the UI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants