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

Flight Icons Migration #20001

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open

Flight Icons Migration #20001

wants to merge 13 commits into from

Conversation

Dhaulagiri
Copy link

This is a start at migrating existing structure icons to FlightIcons. Some caveats that will need to be investigated further by someone more familiar with the codebase

  • There are a handful of x-icon instances left that either take in arguments or didn't have a defined mapping to a new icon when we made the list 2+ years ago. These will need to be looked at on a case-by-case basis.
  • The migration of names between structure and flight was a bit more complicated than I expected. They should be mapped correctly, but if they aren't the component will fail to render.
  • The actual work to remove the structure-icons dependency and x-icon still needs to be done

Copy link

github-actions bot commented Feb 15, 2024

Ember Test Audit comparison

main 2297a43 change
passes 1549 1549 0
failures 0 0 0
flaky 0 0 0
duration 11m 07s 786ms 11m 14s 257ms +06s 471ms

@@ -31,7 +31,7 @@
role="tooltip"
aria-label="Allocation was preempted"
>
{{x-icon "boot" class="is-faded"}}
<FlightIcon @name="boot" class="is-faded" />
Copy link
Contributor

Choose a reason for hiding this comment

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

There is no boot flight icon.

Semantically, the boot here means "preempted" or "evicted" or "kicked out". Maybe there's an adequate Flight replacement.

Copy link
Contributor

Choose a reason for hiding this comment

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

Going to use stop-circle for now until we get a SMB3 goomba boot icon

Copy link
Contributor

Choose a reason for hiding this comment

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

Take my money
image

@Dhaulagiri Dhaulagiri changed the title Flight Icons Migratoin Flight Icons Migration Feb 15, 2024
Copyright (c) HashiCorp, Inc.
SPDX-License-Identifier: BUSL-1.1
~}}

Copy link
Contributor

Choose a reason for hiding this comment

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

SVG code from the formerly-separate .svg file, now as a component. Used in our <LoadingSpinner /> component.

} else if (this.status === 'initializing') {
return 'node-init-circle-fill';
return 'entry-point';
Copy link
Contributor

Choose a reason for hiding this comment

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

Using this for initializing nodes now:
image

@@ -57,3 +57,33 @@ $icon-dimensions-large: 2rem;
color: $grey;
}
}

// Flight Icons general override: notes:
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't feel great about this but it seems necessary given Nomad's current status in a transitory period between a its historical UI design and a more Helios-centered one.

Copy link
Author

Choose a reason for hiding this comment

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

agreed it's not ideal as we would prefer these class names to be more of a private API, however, you're not the first (and won't be the last) to hook into them for specific situations that arise in our products.

@@ -11,7 +11,7 @@
role="tooltip"
aria-label="Allocation depends on unhealthy drivers"
>
{{x-icon "alert-triangle" class="is-warning"}}
<FlightIcon @name="alert-triangle-fill" @color="#fa8e23" />
Copy link
Contributor

Choose a reason for hiding this comment

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

General pattern: bulma classes to add colour vs expressly using the @color prop.

@@ -40,7 +40,6 @@
<TaskGroupRow
@data-test-task-group={{row.model.name}}
@taskGroup={{row.model}}
@onClick={{fn this.gotoTaskGroup row.model}}
Copy link
Contributor

Choose a reason for hiding this comment

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

Side-effect: noticed in testing that clicking the exact centre of task group row's scaling buttons would route you into that task group. Long-term this gets fixed by upgrading to Helios tables.

@tgross tgross added the stage/needs-rebase This PR needs to be rebased on main before it can be backported to pick up new BPA workflows label May 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stage/needs-rebase This PR needs to be rebased on main before it can be backported to pick up new BPA workflows
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants