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

add ui_next dashboard #8402

Merged
merged 8 commits into from
Oct 22, 2020

Conversation

jlmitch5
Copy link
Contributor

@jlmitch5 jlmitch5 commented Oct 15, 2020

Link #8400 #6756

Screen Shot 2020-10-15 at 3 33 45 PM

Screen Shot 2020-10-15 at 3 46 47 PM

This pr adds the dashboard to ui_next. The big changes are:

  • Dashboard counts. These are identical (in terms of values and where they link to) compared to the legacy dashboard.
  • Dashboard graph. This is based off the line chart in Automation Analytics. The UX should be near-100% consistent with AA's chart. In the transition, a few of the filters were lost (namely status-based and hour based time granularity). These will be added once they are supported in AA. Tracking here: Add additional filters to dashboard graph #8401
  • Dashboard lists. These are identical to the full-featured lists on the Jobs and Templates sections of the UI, respectively. The new graphs being full-featured makes portal mode redundant.

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

@@ -316,6 +316,9 @@ def get(self, request, format=None):
if period == 'month':
end_date = start_date - dateutil.relativedelta.relativedelta(months=1)
interval = 'days'
elif period == 'two_weeks':
Copy link
Member

Choose a reason for hiding this comment

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

@ryanpetrello this look good to you? Just want to make sure we're not sneaking something by y'all without a review.

@mabashian
Copy link
Member

Now that we've got recent jobs/templates tabbed out at the bottom my first instinct was... why not have all 3 things in tabs like:

Screen Shot 2020-10-19 at 10 16 56 AM

then there would be little need for scrolling.

Of course, this would mean that recent jobs is now an extra click away. I don't feel strongly about this, it just came to mind as an option. It really depends on how this page is actually used/what users care about seeing.

@trahman73 @wenottingham 👍 / 👎 ? (This could easily be changed in the future as well if we'd rather just stick to the current UX initially.)

jtActions && Object.prototype.hasOwnProperty.call(jtActions, 'POST');
const canAddWFJT =
wfjtActions && Object.prototype.hasOwnProperty.call(wfjtActions, 'POST');
// spreading Set() returns only unique keys
Copy link
Member

Choose a reason for hiding this comment

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

Do we still need this? Not sure which line it's referencing.

@mabashian
Copy link
Member

I think this is looking fantastic @jlmitch5! Curious about ChartTooltip/LineChart in Dashboard/shared. My assumption is that those files are copied over from the other repo. Is that right? Just wondering if we should have a discussion about how to go about sharing components that are not in PF. Keeping distinct copies in each repo means that we have control over it and won't get blindsided by changes needed for AA that may break something we do in AWX. At the same time, we'll have to make sure our copy of these components stays up to date if they change in AA. You may have already talked about this and I'm late to the party but I figure it's worth a conversation. cc @jakemcdermott

@mabashian
Copy link
Member

@unlikelyzero @tiagodread I don't anticipate a tremendous amount of change/refactor on this PR so it's probably safe for you all to start getting familiar with it.

@unlikelyzero
Copy link

@mabashian we are going to do a bug bash on it on wednesday and take a first look at this tomorrow

@unlikelyzero unlikelyzero added the type:feature prioritized on a feature board label Oct 19, 2020
@unlikelyzero
Copy link

Note: we will need to put in //to be implemented for #8401

@trahman73
Copy link

@mabashian That's not a bad idea to combine all 3 views into a tabbed card as shown. I don't think it's a big deal to have to click into the Recent Jobs and Recent Templates tab to get to that content. I'm curious about more opinions as I don't really know how this page is currently used.

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

@jakemcdermott
Copy link
Contributor

jakemcdermott commented Oct 20, 2020

@mabashian

  • I do like the idea of adding the third tab. seems like a nicer ux overall. FWIW I'm for it but curious what @jlmitch5 thinks.
  • The plan was to just duplicate the chart code over to awx and not worry about making a shared repo or lib at this time. I think we should stick to this plan. IMO there's no need to dive into the complexity of cross-project code sharing until after 4.0
  • I heard something about having to move to victory charts at some point. That seems... non-essential as a first step and again would prefer post 4.0 unless it's a blocker for some reason.

@softwarefactory-project-zuul
Copy link
Contributor

Build failed.

@unlikelyzero
Copy link

unlikelyzero commented Oct 21, 2020

@trahman73 @jlmitch5 @jakemcdermott the 3 tab design also improves client-side performance. Our weighted lighthouse score improved by 3/100 points between revisions

@unlikelyzero
Copy link

unlikelyzero commented Oct 21, 2020

@jlmitch5 A bug-free PR!

  • will the Chart Hoverover text be translatable?

The only point of feedback is that we'd like to have this a bit more testable.

  • Can you work with us to come up with a deterministic way of hovering over a datapoint? There are no tests for the AA version of this chart so this might involve some minor changes
  • Could you add unique IDs to the Cards and chart filter dropdowns

We have an associated PR that we'd like to have merged alongside this PR. It should contain:

  • Any associated testability improvements.
  • All stubs which were executed manually
  • All stubs for future work with //to be implemented
  • A visual test for the Dashboard and Graph
  • A single test to validate the correctness of the cards.
  • A single test to validate the correctness of the chart hoverover
  • Re-used tests for CRUD and Search on Job List
  • Re-used tests for CRUD and Search on Recent Templates

@jakemcdermott
Copy link
Contributor

recheck

@softwarefactory-project-zuul
Copy link
Contributor

Build failed.

@mabashian
Copy link
Member

^^ I think this is a legitimate failure

2020-10-21 12:49:09.462583 | static | FAIL src/screens/Dashboard/Dashboard.test.jsx
2020-10-21 12:49:09.462961 | static |   ● <Dashboard /> › renders jobs list by default
2020-10-21 12:49:09.462997 | static |
2020-10-21 12:49:09.463381 | static |     expect(received).toBe(expected) // Object.is equality
2020-10-21 12:49:09.463416 | static |
2020-10-21 12:49:09.463505 | static |     Expected: 1
2020-10-21 12:49:09.463677 | static |     Received: 0
2020-10-21 12:49:09.463713 | static |
2020-10-21 12:49:09.463762 | static |       31 |
2020-10-21 12:49:09.464156 | static |       32 |   test('renders jobs list by default', () => {
2020-10-21 12:49:09.464786 | static |     > 33 |     expect(pageWrapper.find('JobList').length).toBe(1);
2020-10-21 12:49:09.465935 | static |          |                                                ^
2020-10-21 12:49:09.466021 | static |       34 |   });
2020-10-21 12:49:09.466059 | static |       35 |
2020-10-21 12:49:09.466184 | static |       36 |   test('renders template list when the active tab is changed', async () => {
2020-10-21 12:49:09.466211 | static |
2020-10-21 12:49:09.466396 | static |       at Object.test (src/screens/Dashboard/Dashboard.test.jsx:33:48)

Test probably need to be updated after changing to the 3 tabs view

@softwarefactory-project-zuul
Copy link
Contributor

Build failed.

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

@unlikelyzero
Copy link

@jlmitch5 we're almost done with the tests. We just need unique IDs for the tabs and the cards. Is that something you can get in tomorrow?

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

@unlikelyzero
Copy link

Associated Tests in 5622

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded (gate pipeline).

@softwarefactory-project-zuul softwarefactory-project-zuul bot merged commit 466dff9 into ansible:devel Oct 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:api type:feature prioritized on a feature board
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants