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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[馃毀 POC: not for merge] @hashicorp/ember-flight-icons test #12533

Closed
wants to merge 2 commits into from

Conversation

amyrlam
Copy link
Contributor

@amyrlam amyrlam commented Sep 11, 2021

Quick proof of concept (POC) of https://www.npmjs.com/package/@hashicorp/ember-flight-icons in Vault

Thanks to Michael Lange's help, I ended up downloading Vault from https://www.vaultproject.io/ and running vault server -dev along with cd ui && yarn start to run local dev. (Previously got stuck on various dev env woes, but am chipping away at those!)

Tagging Vault UI eng & design, as well as some of the Design Systems team for reference.

image

  • TODO: Next steps would be to get this queued on the Vault Jira board, for when you have bandwidth, I'll xpost in Slack

@@ -106,3 +108,7 @@
margin: 0 $spacing-xs;
width: 0;
}

.toolbar-button-spacer {
margin-left: 4px; // Note: We should give a reco what spacing we want, when inline with text
Copy link
Contributor Author

Choose a reason for hiding this comment

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

wanted to crosspost from slack convo to explain what i meant:

  • the structure-icons included some padding as part of the .svg file. the flight icons do not include padding within the .svg
  • i added 4px horizontal spacing between the default 16 size flight icon and adjacent inline text, which i think looks fine
  • eventually we should bring those recommendations to the documentation of flight https://github.com/hashicorp/flight (link in the upper RHS, just sharing the repo in case the vercel link changes)

@@ -47,6 +47,7 @@
"@ember/render-modifiers": "^1.0.2",
"@glimmer/component": "^1.0.2",
"@glimmer/tracking": "^1.0.2",
"@hashicorp/ember-flight-icons": "^0.0.2-beta",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

update: we're at a 1.0.0 now and this could be updated!

but will wait to make further changes til y'all are working on it (no rush on our end)

Copy link
Collaborator

@hashishaw hashishaw left a comment

Choose a reason for hiding this comment

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

This is an excellent POC @amyrlam, thank you for putting it together!

I'm pleased that the changes don't seem very drastic, although the considerations top of mind as we take this forward are:

  • mapping between old glyph names and new ones (this may involve manual testing, or maybe we can extend FlightIcon with an assertion about @name value being valid. Does that already exist?)
  • I'm sure there are a million little places where we handle icon spacing -- I wonder if there's a better systematic way to do that 馃

@amyrlam
Copy link
Contributor Author

amyrlam commented Sep 27, 2021

@chelshaw hey welcome back! there's not a firm timeline on this, but it would be nice to chip away at deprecating structure-icons, esp if new designs with icons come up.

here's a list of mappings in the repo: https://github.com/hashicorp/flight/blob/main/structure-mappings.json

there has been exploration of a codemod, but because many of the structure-icons included padding as part of the .svg, it's not something i've explored that much. that being said, let me look more into it. looks like the WIP of that is here hashicorp/flight#141

@amyrlam amyrlam added the ui label Oct 13, 2021
@amyrlam
Copy link
Contributor Author

amyrlam commented Dec 7, 2021

Close in favor of #12976, thank you @zofskeez!

@amyrlam amyrlam closed this Dec 7, 2021
@amyrlam amyrlam deleted the amy/ember-flight-icons-test branch December 7, 2021 00:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants