ui: update icons to use the FlightIcon package#2681
Conversation
|
Just wanted to update on GH as well but this is on hold until we get the HC logos in Flight |
8f75f55 to
93aa454
Compare
Ember Asset Size actionAs of f82b213 Files that got Bigger 🚨:
Files that got Smaller 🎉:
|
93aa454 to
b0e9ac1
Compare
jgwhite
left a comment
There was a problem hiding this comment.
The new icons are looking really nice.
I spotted a few nitpicks, but in general the transition is butter smooth.
gregone
left a comment
There was a problem hiding this comment.
LGTM. just a few questions about using the button element for styling
didoo
left a comment
There was a problem hiding this comment.
@sabrinako added a few comments, take them for what they are (just suggestions)
|
|
||
| &--external-link { | ||
| > .pds-icon { | ||
| > svg { |
There was a problem hiding this comment.
here you're relying on the component internal structure (expects the SVG to be a direct child) and in case we decide in the future to change it, this would break (unknowingly) the styling.
my suggestion is to use a wrapping container/div, and style that container. in this way you decouple hosting codebase from the components implementation
| align-items: stretch; | ||
| margin-left: calc(-1 * 0.25rem); | ||
|
|
||
| svg { |
There was a problem hiding this comment.
potentially, also here you have a coupling (less strong than the other one, but still)
one option could be to apply a class to the FlightIcon component and use that one as a CSS selector here (and also in other cases I see where you have used the DOM element as target)
| height: scale.$lg-14; | ||
|
|
||
| path { | ||
| use { |
There was a problem hiding this comment.
this is not how we would expect color to be applied. you're manipulating directly the internal SVG but we expose a @color prop for this (see https://flight-hashicorp.vercel.app/engineering#example-fill)
plus, our icons use currentColor by default so they can just inherit the color from the parent container
plus, our icons are flat, they don't contain strokes, so the stroke: $brand-color; declaration can be removed
|
|
||
| .icon.brand { | ||
| display: flex; | ||
| width: scale.$lg-14; |
There was a problem hiding this comment.
@heatherlarsen for knowledge, one case in which the 24px icon is scaled up to 96px
There was a problem hiding this comment.
I ran into an issue getting this to run locally, but I'm assuming this is either for the Waypoint logo or the HashiCorp logo. I'd advocate for using a logo in these instances instead of an icon.
There was a problem hiding this comment.
I ran into an issue getting this to run locally
@heatherlarsen I’d like to hear more about this. I’ll reach out to you on Slack :)
| .pds-icon { | ||
| svg { | ||
| position: absolute; | ||
| font-size: scale.$lg--1; |
|
@sabrinako I saw your note about changing the stroke weight. Stroke weight is not something that should be changed on Flight icons as each icon was intentionally designed with consistent stroke weight based on the size of the icon. More information about how the icons are drawn can be found in the docs. Is there a specific reason you need to adjust the stroke weight? |

Related Issue
#2392
Description
This PR intends to transfer the Waypoint Ui over from using the
Pds::Iconcomponents to the newFlightIconones.How to Test
git checkout ui/flight-icon-updateyarnto download the new packagesyarn startto boot up UI with mock dataAdditional Notes
flight-iconscurrently doesn't have any animated icons, so ourloadingspinner is not present. There's discussion about adding it that's picked up in recent days, so I imagine this will be added soon but I'm not planning on holding this PR back any longer since it's just 1 icon left.Screenshots
Deployments Page
Auth Page
As of making this PR, the method for altering the colors of the icons in
Flightrequires altering theuseelement instead ofpath. This also seems to prevent us from changing the stroke width. Feedback on how we could change the stroke width would be helpful (since I'm pretty sure it was thinner before)