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: add links to legend items in allocation-summary #11820

Merged
merged 8 commits into from
Jan 14, 2022
Merged

feat: add links to legend items in allocation-summary #11820

merged 8 commits into from
Jan 14, 2022

Conversation

ChaiWithJai
Copy link
Contributor

Actual

image

@ChaiWithJai ChaiWithJai linked an issue Jan 11, 2022 that may be closed by this pull request
Add clickable links in allocation summary #11442
Closed
LevelbossMike
LevelbossMike approved these changes Jan 11, 2022
View reviewed changes
Copy link
Contributor

@LevelbossMike LevelbossMike left a comment

Choose a reason for hiding this comment

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

this looks good to me 👏

@github-actions
Copy link

github-actions bot commented Jan 11, 2022 •
edited
Loading

Ember Asset Size action

As of 9731dea

Files that got Bigger 🚨:

File raw gzip
nomad-ui.js +2.11 kB +175 B

Files that got Smaller 🎉:

File raw gzip
nomad-ui.css -152 B +51 B

Files that stayed the same size 🤷‍:

File raw gzip
vendor.js 0 B 0 B
vendor.css 0 B 0 B

@github-actions
Copy link

github-actions bot commented Jan 11, 2022 •
edited
Loading

Ember Test Audit comparison

main 9731dea change
passes 1245 1258 +13
failures 0 1 +1
flaky 0 0 0
duration 11m 44s 530ms 000ms -11m 44s 530ms

@github-actions
Copy link

Ember Test Audit flaky tests

Ember Test Audit detected these flaky tests on main:

  • Acceptance | job dispatch (with namespace): required meta fields are properly indicated

lgfa29
lgfa29 approved these changes Jan 12, 2022
View reviewed changes
Copy link
Contributor

@lgfa29 lgfa29 left a comment

Choose a reason for hiding this comment

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

Just missing a changelog entry.

Also, it would good if there were a quick test around this new clicking behaviour.

ui/app/templates/components/job-page/parts/summary-legend-item.hbs
Comment on lines -2 to +11
<span class="color-swatch {{if @datum.className @datum.className (concat "swatch-" @index)}}" />
<span
class="color-swatch {{if @datum.className @datum.className (concat "swatch-" @index)}}"
></span>
<span class="text">
<span class="value" data-test-legend-value="{{@datum.className}}">{{@datum.value}}</span>
<span class="label">{{@datum.label}}</span>
<span class="value" data-test-legend-value="{{@datum.className}}">
{{@datum.value}}
</span>
<span>
{{@datum.label}}
</span>
Copy link
Contributor

Choose a reason for hiding this comment

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

Auto-formater strikes again 😄

I think this file didn't actually change?

@vercel vercel bot temporarily deployed to Preview – nomad January 12, 2022 16:24 Inactive
@github-actions
Copy link

Ember Test Audit flaky tests

Ember Test Audit detected these flaky tests on 045ff73:

  • Acceptance | task group detail: /jobs/:id/:task-group second breadcrumb should link to the job for the task group
  • Integration | Component | TopoViz: clicking on an allocation in a deeply nested TopoViz::Node will associate sibling allocations with curves
  • Integration | Component | TopoViz: when the count of sibling allocations is high enough relative to the node count, curves are not rendered
  • Integration | Utility | exec-socket-xterm-adapter: resizing the window passes a resize message through the socket

lgfa29
lgfa29 reviewed Jan 13, 2022
View reviewed changes
ui/app/styles/charts/distribution-bar.scss
@@ -98,8 +97,6 @@
&.is-clickable {
a {
display: block;
text-decoration: none;
color: inherit;
}

&:not(.is-empty) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we can remove this rule now? The background colour change on hover may not be necessary anymore.

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thought:

Let's wait on styling and UX changes until the Nomad Design Refresh slated for 2.0. Otherwise, we'll go back and forth on changes for a long time when I can prioritize other higher value activities in the meantime. Also... we don't have visual regression testing in place, I'd rather focus on getting that up prior to making changes so we don't get blindsided by changes.

@vercel vercel bot temporarily deployed to Preview – nomad January 14, 2022 15:30 Inactive
@ChaiWithJai ChaiWithJai merged commit fcd86e4 into main Jan 14, 2022
@ChaiWithJai ChaiWithJai deleted the f-ui/alloc-legend branch January 14, 2022 19:02
@github-actions
Copy link

github-actions bot commented Nov 3, 2022

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 3, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@lgfa29 lgfa29 lgfa29 approved these changes

@LevelbossMike LevelbossMike LevelbossMike approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add clickable links in allocation summary
3 participants
@ChaiWithJai @LevelbossMike @lgfa29