-
Notifications
You must be signed in to change notification settings - Fork 2k
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
UI: Collapsable job summary visualization #4504
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, I like the different tooltip view when it's collapsed aswell. Just noticed a couple of tests are failing now? From the titles they look possibly unrelated, but also just noticed Travis caught them also
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice feature!
return wait(); | ||
}) | ||
.then(() => { | ||
assert.notOk(window.localStorage.nomadExpandJobSummary, 'No value in localStorage yet'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is going to result in flaky tests due to localStorage state retention. We initially worked around this in TFE by wiping localStorage in our moduleForAcceptance
helper, but eventually switched to ember-window-mock.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I clear localStorage
in the beforeEach for every integration test that depends on it (and in moduleForAcceptance
as well) and haven't run into any problems.
I'll look into ember-window-mock though. It's good to be consistent across codebases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just my 2 cents here, I'd use mocking in unit or integration tests, but if its not complicated to use the real thing in an acceptance test then I would. All browsers have localStorage so I'd say ideally it'd be used during acceptance - just my take.
onClose=(action (queue | ||
(action (mut item.isOpen) false) | ||
(action onToggle item.item item.isOpen) | ||
)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👌
Yep, those test failures were due to parallel branches being merged. Breadcrumbs didn't account for the evaluations page and the evaluations page didn't account for breadcrumbs since they were both being worked on at the same time. I fixed that all in the PageObjects branch that just merged. I'll rebase this branch and Travis should go green. |
9b879c9
to
af14f72
Compare
top notch! |
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. |
The job summary page can get awfully long, so this is one way to get to stuff (e.g., task group links) quicker.
When collapsed, the "inline" version of the chart is used, when expanded, the experience is the same as it is today.
Your preference is persisted to localStorage, so it should more or less be a set-it-and-forget-it situation.
Looks like this 👇