Refactor: Breadcrumbs Service #11590

ChaiWithJai · 2021-11-30T20:23:23Z

Background

Nomad and Waypoint both share a similar implementation of the Breadcrumbs service. Breadcrumbs are a navigation UI pattern to help users easily understand where they are when they’re inside of a deeply nested route.

Currently, Nomad and Waypoint use a Breadcrumbs service that uses the Router service to access the currentRoute and currentUrl and creates a computed derived state of a breadcrumbs array that is consumed by an AppBreadcrumbs component.

This pattern requires that Nomad and Waypoint both store state on the router. This breaks the Router/Controller boundary and the current use of the Router service breaks the Data Down, Actions Up paradigm by directly injecting the route into a service and passing the information into a component.

A Different Solution

Instead of violating the boundary between controllers and routers, we can use Renderless Components that consume our Breadcrumbs Service.

Our Breadcrumbs Service will be responsible for storing our breadcrumbs along with 2 functions: registering and deregistering breadcrumbs.

All Routes have templates that are rendered while the route is active, we can use a renderless component that consumes the Breadcrumbs Service and take advantage of 2 lifecycle methods: did-insert and will-destroy.

Whenever, a Route is initialized the component will intialize and register its trail of breadcrumbs to the breadcrumbs service. If we navigate away from the route, the component will unmount and unregister from our breadcrumbs trail.

Prior Art

We already follow this pattern in HCP

github-actions · 2021-11-30T20:30:36Z

Ember Asset Size action

As of 3bdf661

Files that got Bigger 🚨:

File	raw	gzip
nomad-ui.js	+18.7 kB	+2.37 kB
vendor.js	+1.57 kB	+249 B
nomad-ui.css	+204 B	+61 B

Files that stayed the same size 🤷‍:

File	raw	gzip
vendor.css	0 B	0 B

ChaiWithJai · 2021-12-15T17:36:19Z

ui/app/controllers/jobs/job/task-group.js

-    Searchable,
-    WithNamespaceResetting
-  ) {
+  /* eslint-disable indent */


Creating a follow-up task for this. Not valuable to tackle now.

https://github.com/prettier/eslint-config-prettier would probably help with this.

github-actions · 2021-12-15T17:41:46Z

Ember Test Audit comparison

	main	`3bdf661`	change
passes	1238	1235	-3
failures	0	1	+1
flaky	0	1	+1
duration	10m 37s 007ms	000ms	-10m 37s 007ms

github-actions · 2021-12-15T17:41:48Z

Ember Test Audit flaky tests

Ember Test Audit detected these flaky tests on 8a29d25:

Acceptance | task group detail: /jobs/:id/:task-group should list high-level metrics for the allocation
Acceptance | task group detail: /jobs/:id/:task-group second breadcrumb should link to the job for the task group

github-actions · 2021-12-15T18:17:23Z

Ember Test Audit flaky tests

Ember Test Audit detected these flaky tests on main:

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

jgwhite · 2021-12-16T15:39:34Z

@ChaiWithJai just to help us (@hashicorp/pl-applications-frontend) understand how to prioritize review for this:

Is this blocking other work?
Are there other motivations/priorities we should be aware of? (other works going on at the same time, that it might interact with)

jgwhite · 2021-12-16T15:42:11Z

Copying from a Slack conversation with @ChaiWithJai, for reference:

Important commits:

Breadcrumb Service (Register/Deregister Pattern) 5f9fc61

Data Loading Component 5a1df04

Handle async relationships and remove need to resolve PromiseProxys ab460da

ChaiWithJai · 2021-12-16T15:46:15Z

@ChaiWithJai just to help us (@hashicorp/pl-applications-frontend) understand how to prioritize review for this:

Is this blocking other work?

Are there other motivations/priorities we should be aware of? (other works going on at the same time, that it might interact with)

@jgwhite this is blocking 11436.

lgfa29

Great job on the refactoring!

I added some small comments and knit-picks, but the main ones are:

the is-active class got dropped from the active crumb
the a11y lint rule should still be set and used in tests
the auto-format in some of the template files made them a little weird and harder to read, I think it would be worth undoing those changes but I'm interesting in hearing what others think.

ui/app/components/breadcrumbs/job.hbs

ui/app/controllers/csi/plugins/plugin.js

lgfa29 · 2021-12-16T20:35:16Z

ui/app/controllers/jobs/job/task-group.js

-    Searchable,
-    WithNamespaceResetting
-  ) {
+  /* eslint-disable indent */


https://github.com/prettier/eslint-config-prettier would probably help with this.

ui/app/services/breadcrumbs.js

lgfa29 · 2021-12-16T21:19:57Z

ui/app/templates/csi/volumes/volume.hbs

-            <th>ID</th>
-            <th>Created</th>
-            <th>Modified</th>
-            <th>Status</th>
-            <th>Client</th>
-            <th>Job</th>
-            <th>Version</th>
-            <th>CPU</th>
-            <th>Memory</th>
+            <th>
+              ID
+            </th>
+            <th>
+              Created
+            </th>
+            <th>
+              Modified
+            </th>
+            <th>
+              Status
+            </th>
+            <th>
+              Client
+            </th>
+            <th>
+              Job
+            </th>
+            <th>
+              Version
+            </th>
+            <th>
+              CPU
+            </th>
+            <th>
+              Memory
+            </th>


I find that this (and other table header changes) make the file harder to read. Is this something that can be disabled in your code formater?

I have major issues with my eslint and prettier config. I'm planning to tackle this in one swoop later. But, I can spend the day tomorrow and figure this out.

Cool, let me know if you need help. It may be easier to do it with an interactive rebase with git than manually revert them.

lgfa29 · 2021-12-16T21:35:20Z

ui/app/controllers/servers/server.js

+  get breadcrumb() {
+    return {
+      label: this.server.name,
+      args: ['servers.server', this.server.id],
+    };
+  }
+}


Is there a criteria fro when to use a list vs. a single object? The other controllers seem to use a list even if there's only one item. Perhaps this is something that should be standardized

There's no criteria of when to use a single object versus a list. We use a list when we're in a route that wants to show that it is part of a hierarchy that it's not actually a part of in router.js.

Generally, that's the only use case for using a list. Everything else should render to depict the route's model.

Yeah... I see the inconsistencies. Gonna re-do all the commits and clean this up tomorrow.

lgfa29 · 2021-12-16T21:37:16Z

ui/tests/.eslintrc.js

-      rules: {
-        'ember-a11y-testing/a11y-audit-called': 'error',
-      },


I think we want to keep this, and only disable in specific cases.

Hey, great callout. Upon examining this package in our package.json this package is pointing to a commit in a repo and not the npm registry. No other HashiCorp product has this pattern. Its worth creating a follow-up issue to edit this behavior and be in line with HashiCorp's a11y standards.

Currently, this rule creates a lot of noise in our tests.

lgfa29 · 2021-12-16T21:39:01Z

ui/tests/acceptance/task-detail-test.js

+
+    await waitFor('[data-test-job-breadcrumb]');


Maybe move this up to keep all related assert close together.

lgfa29 · 2021-12-16T21:41:26Z

ui/tests/integration/components/app-breadcrumbs-test.js

    );

-    await componentA11yAudit(this.element, assert);


This is probably why the linter was complaining.

ChaiWithJai · 2021-12-17T16:36:48Z

ui/app/styles/core/breadcrumb.scss

+    font-size: x-large;
+  }
+
+  li:last-child a.active {


The LinkTo component is connected to the route layer and gets an active property if the route is active.

We use last-child if we're in a nested route because the parent route will also be set to active.

There's a namespace related issue where if the namespace query parameter is not forwarded then LinkTo will not be set to active. This is the same root cause as noted in 11327 and is noted by the Ember Core team.

We're watching the issue and all links will be properly highlighted when the issue is resolved by Ember Core and we don't think its worthwhile to create a workaround solution as this will be fixed by Core soon.

All breadcrumbs do not need a title property because some views drill down by using a tab-based UI (e.g. CSI volumes and the Job Overview) The goal is to help us identify breadcrumbs that are non-descriptive (i.e. breadcrumbs that display as an ID).

github-actions · 2021-12-23T18:25:27Z

Ember Test Audit flaky tests

Ember Test Audit detected these flaky tests on 3bdf661:

Acceptance | task group detail: /jobs/:id/:task-group should list high-level metrics for the allocation

lgfa29

Nice improvement! Just for reference, code formatting changes will be handled in #11754

github-actions · 2022-11-05T02:33:38Z

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.

vercel bot temporarily deployed to Preview – nomad December 8, 2021 19:26 Inactive

vercel bot deployed to Preview – nomad-storybook-and-ui December 8, 2021 19:27 View deployment

vercel bot temporarily deployed to Preview – nomad December 9, 2021 16:33 Inactive

vercel bot deployed to Preview – nomad-storybook-and-ui December 9, 2021 16:33 View deployment

vercel bot temporarily deployed to Preview – nomad December 14, 2021 19:46 Inactive

vercel bot deployed to Preview – nomad-storybook-and-ui December 14, 2021 19:46 View deployment

vercel bot deployed to Preview – nomad-storybook-and-ui December 15, 2021 16:56 View deployment

vercel bot temporarily deployed to Preview – nomad December 15, 2021 16:56 Inactive

ChaiWithJai changed the title ~~Exploratory Refactor: Breadcrumbs Service~~ Refactor: Breadcrumbs Service Dec 15, 2021

ChaiWithJai marked this pull request as ready for review December 15, 2021 16:58

vercel bot temporarily deployed to Preview – nomad December 15, 2021 17:04 Inactive

vercel bot deployed to Preview – nomad-storybook-and-ui December 15, 2021 17:04 View deployment

ChaiWithJai commented Dec 15, 2021

View reviewed changes

vercel bot deployed to Preview – nomad-storybook-and-ui December 15, 2021 17:37 View deployment

vercel bot temporarily deployed to Preview – nomad December 15, 2021 17:37 Inactive

ChaiWithJai requested a review from a team December 15, 2021 17:47

ChaiWithJai requested review from lgfa29 and DingoEatingFuzz December 15, 2021 22:21

lgfa29 suggested changes Dec 16, 2021

View reviewed changes

vercel bot deployed to Preview – nomad-storybook-and-ui December 17, 2021 16:29 View deployment

vercel bot temporarily deployed to Preview – nomad December 17, 2021 16:29 Inactive

ChaiWithJai commented Dec 17, 2021

View reviewed changes

vercel bot temporarily deployed to Preview – nomad December 17, 2021 16:49 Inactive

ChaiWithJai added 12 commits December 21, 2021 07:46

fix: update breadcrumb tests according to new breadcrumb structure

5ca9ebe

fix: add in for topology route

fa5096a

fix: delete jobs route - ember creates this for us

dd04270

update: add comment to explain loading namespaces in alloc route

7bf2880

feat: add title to default breadcrumb component

0334709

style: add styling for title on breadcrumbs

a262979

feat: handle title behavior for job breadcrumb

10db60e

style: centering and spacing for titled breadcrumbs

ed49d59

fix: test specs should expect to receive breadcrumb titles

1a2914b

styling: fix opacity for last child in the list

92fafca

fix: add ember-a11y-testing-audit-call

4d39d88

ChaiWithJai force-pushed the e-ui/breadcrumbs-service branch from 851db8e to 4d39d88 Compare December 21, 2021 12:47

vercel bot had a problem deploying to Preview – nomad-storybook-and-ui December 21, 2021 12:47 Failure

vercel bot temporarily deployed to Preview – nomad December 21, 2021 12:47 Inactive

chore: clean-up merge conflict after rebase

c1d2920

vercel bot deployed to Preview – nomad-storybook-and-ui December 21, 2021 13:06 View deployment

vercel bot temporarily deployed to Preview – nomad December 21, 2021 13:06 Inactive

ChaiWithJai added 2 commits December 23, 2021 12:33

fix: remove unecessary breadcrumb

b25f5ff

refact: clean-up breadcrumb invocations

3bdf661

vercel bot temporarily deployed to Preview – nomad December 23, 2021 17:43 Inactive

vercel bot deployed to Preview – nomad-storybook-and-ui December 23, 2021 17:43 View deployment

lgfa29 approved these changes Jan 5, 2022

View reviewed changes

ChaiWithJai merged commit 38a3759 into main Jan 5, 2022

ChaiWithJai deleted the e-ui/breadcrumbs-service branch January 5, 2022 22:46

This was referenced Jan 6, 2022

ui: fix CI #11785

Merged

UI: add name for the job breadcrumbs to help users understand the hierarchy #11436

Closed

github-actions bot locked as resolved and limited conversation to collaborators Nov 5, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor: Breadcrumbs Service #11590

Refactor: Breadcrumbs Service #11590

ChaiWithJai commented Nov 30, 2021

github-actions bot commented Nov 30, 2021 •

edited

Loading

ChaiWithJai Dec 15, 2021

lgfa29 Dec 16, 2021

github-actions bot commented Dec 15, 2021 •

edited

Loading

github-actions bot commented Dec 15, 2021

github-actions bot commented Dec 15, 2021

jgwhite commented Dec 16, 2021

jgwhite commented Dec 16, 2021

ChaiWithJai commented Dec 16, 2021

lgfa29 left a comment

lgfa29 Dec 16, 2021

lgfa29 Dec 16, 2021

ChaiWithJai Dec 16, 2021

lgfa29 Dec 16, 2021

lgfa29 Dec 16, 2021

ChaiWithJai Dec 16, 2021

ChaiWithJai Dec 16, 2021

lgfa29 Dec 16, 2021

ChaiWithJai Dec 17, 2021

lgfa29 Dec 16, 2021

lgfa29 Dec 16, 2021

ChaiWithJai Dec 17, 2021

github-actions bot commented Dec 23, 2021

lgfa29 left a comment

github-actions bot commented Nov 5, 2022

Refactor: Breadcrumbs Service #11590

Refactor: Breadcrumbs Service #11590

Conversation

ChaiWithJai commented Nov 30, 2021

Background

A Different Solution

Prior Art

github-actions bot commented Nov 30, 2021 • edited Loading

Ember Asset Size action

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Dec 15, 2021 • edited Loading

Ember Test Audit comparison

github-actions bot commented Dec 15, 2021

Ember Test Audit flaky tests

github-actions bot commented Dec 15, 2021

Ember Test Audit flaky tests

jgwhite commented Dec 16, 2021

jgwhite commented Dec 16, 2021

ChaiWithJai commented Dec 16, 2021

lgfa29 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Dec 23, 2021

Ember Test Audit flaky tests

lgfa29 left a comment

Choose a reason for hiding this comment

github-actions bot commented Nov 5, 2022

github-actions bot commented Nov 30, 2021 •

edited

Loading

github-actions bot commented Dec 15, 2021 •

edited

Loading