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

QA dashboard overview FE #2329

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

Conversation

AdamAdHocTeam
Copy link
Collaborator

@AdamAdHocTeam AdamAdHocTeam commented Aug 27, 2024

Description of change

This is the frontend for the QA dashboard overview section. It has three widgets and should show the new 'Filter not applied' message and tool tip when the filters aren't applicable (currently hard coded).

How to test

  • Review the code.
  • View the QA dashboard we should have a review populated with dummy data.
  • Make sure we match the mockups.

Issue(s)

Checklists

Every PR

  • Meets issue criteria
  • JIRA ticket status updated
  • Code is meaningfully tested
  • Meets accessibility standards (WCAG 2.1 Levels A, AA)
  • API Documentation updated
  • Boundary diagram updated
  • Logical Data Model updated
  • Architectural Decision Records written for major infrastructure decisions
  • UI review complete

Before merge to main

  • OHS demo complete
  • Ready to create production PR

Production Deploy

  • Staging smoke test completed

After merge/deploy

  • Update JIRA ticket status

@thewatermethod
Copy link
Collaborator

These are like twice the size of the mockup :(

Screenshot 2024-08-28 at 10 40 09 AM

import Tooltip from '../components/Tooltip';
import colors from '../colors';

export function Field({
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we please reuse existing components here, and just add a prop to handle the differences? that way we wouldn't need a whole new CSS file/reinvent the wheel

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yea I added a new component and used it in both places.

@@ -0,0 +1,47 @@
@media( min-width: 640px){
Copy link
Collaborator

Choose a reason for hiding this comment

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

would it be possible to reuse the HTML/CSS from the existing widgets and just override stuff here where necessary, rather than recoding this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have a new CSS.

@thewatermethod
Copy link
Collaborator

Thanks for the cleanup. Can we go further and have one overview widget component (maybe start with the one on the regional dashboard?

Also, the icon size and some of the text in the overview widgets are pretty different from the mockup, it'd be good if we could keep the sizes and stuff consistent with regional dashboard.

import Tooltip from '../components/Tooltip';
import colors from '../colors';

export function OverviewWidgetField({
Copy link
Collaborator

Choose a reason for hiding this comment

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

There's a duplicative component in frontend/src/widgets/DashboardOverview.js, can we condense

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Will leave as is for now as this will need to be updated to use the new overview widget in the future.

Copy link
Collaborator

@thewatermethod thewatermethod Sep 4, 2024

Choose a reason for hiding this comment

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

can you explain what you mean? @AdamAdHocTeam

@@ -0,0 +1,51 @@
@media( min-width: 1024px ){
Copy link
Collaborator

Choose a reason for hiding this comment

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

There's duplicative CSS at frontend/src/widgets/DashboardOverview.css, if we condense the components we can probably condense the CSS as well

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This widget will need to be updated to use the new overview widget when we do this it will go away. I have only updated resource and qa dashboard atm.

Copy link
Collaborator

@thewatermethod thewatermethod Sep 4, 2024

Choose a reason for hiding this comment

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

what is the new overview widget? @AdamAdHocTeam

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So we have a overview widget container now that wraps around all the individual overview widgets. I think this needs to be updated to that structure.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We don't need to pass array of field components just an array of object info.

Comment on lines 19 to 25
.smart-hub--overview-widget-field-font-size {
font-size: 15px;
}

.smart-hub--overview-widget-field-data-font-size {
font-size: 19px;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we use the uswds font size classes for these, please?

Copy link
Collaborator

Choose a reason for hiding this comment

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

any luck on this change ^ @AdamAdHocTeam

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@thewatermethod oops missed this looking now

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

should be updated now.

Comment on lines 1 to 12
@media( min-width: 640px){
.smart-hub--dashboard-overview-container {
column-gap: 1rem;
row-gap:.5rem;
}
}

@media( min-width: 1024px ){
.smart-hub--dashboard-overview-container {
gap: 1em;
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggestion:

  1. Delete this file.
  2. Remove "margin-bottom-1" from the classnames OverviewWidgetField line 29
  3. add "flex-gap-2" to the classnames in "DashboardOverviewContainer" line 32

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

changes made.

Comment on lines 7 to 17
.smart-hub--dashboard-overview-widget-field-icon-background-lg {
border-radius: 50%;
height: 5em;
width: 5em;
}

.smart-hub--dashboard-overview-widget-field-icon-background-sm {
border-radius: 50%;
height: 3em;
width: 3em;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Added suggestion, would need to be coupled with adjustments in the HTML

Suggested change
.smart-hub--dashboard-overview-widget-field-icon-background-lg {
border-radius: 50%;
height: 5em;
width: 5em;
}
.smart-hub--dashboard-overview-widget-field-icon-background-sm {
border-radius: 50%;
height: 3em;
width: 3em;
}
.smart-hub--dashboard-overview-widget-field-icon {
border-radius: 50%;
}
.smart-hub--dashboard-overview-widget-field-icon__background-lg {
height: 5em;
width: 5em;
}
.smart-hub--dashboard-overview-widget-field-icon__background-sm {
height: 3em;
width: 3em;
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

updated.

Comment on lines 49 to 50
height: 24px;
width: 24px;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we switch to em to match the other heights in the file?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I switched to em.

thewatermethod and others added 30 commits September 10, 2024 15:30
…ord' into mb/TTAHUB-3345/filters-on-QA-dashboard
…ord' into mb/TTAHUB/3345/add-filters-to-qa-dash
[TTAHUB-3305, 3306] QA dashboard overview detail pages for first two widgets
…ets into mb/TTAHUB/3345/add-filters-to-qa-dash
[TTAHUB-3345] Add filters to QA dashboard, frontend
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants