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

Convert JobList to PaginatedTable #8970

Merged
merged 7 commits into from
Jan 27, 2021
Merged

Convert JobList to PaginatedTable #8970

merged 7 commits into from
Jan 27, 2021

Conversation

keithjgrant
Copy link
Member

@keithjgrant keithjgrant commented Dec 22, 2020 •
edited
Loading

SUMMARY

Converts JobList component to use PaginatedTable. I also added the expandable content rows while I was in there

addresses #6189

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME
  • UI

Screen Shot 2020-12-23 at 2 34 21 PM

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

@keithjgrant keithjgrant force-pushed the 6189-job-list-tables branch from 8411f6a to 2abcc8e Compare January 6, 2021 19:19
@softwarefactory-project-zuul
Copy link
Contributor

Merge Failed.

This change or one of its cross-repo dependencies was unable to be automatically merged with the current state of its repository. Please rebase the change and upload a new patchset.

@keithjgrant keithjgrant force-pushed the 6189-job-list-tables branch from 2abcc8e to fc775df Compare January 6, 2021 19:22
@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

@nixocio nixocio self-requested a review January 7, 2021 16:30
@tiagodread
Copy link
Contributor

Hey @keithjgrant I've verified that the user can:

  • navigate to job details
  • relaunch a job
  • use the basic and advanced search
  • delete a single or some finished jobs
  • cancel a single or some running jobs
  • sort jobs by name, status, type, start time and finish time
  • change the number of job items on the list
  • paginate through job items
  • expand/collapse the row
  • The loading state component is displayed

Responsive tests against:
1024x768 and 1920x1080

Considerations:
Are we going to cover these launched by and Project sorting options?
image

If the user selects a running/pending and a successful job the delete button is enabled and trying to delete both:

image

Are these data Started Time and Finish Time on the and expandable-row the same data or they are duplicate?

image

Could you add a data-ouia-component-id (when available) or IDs on:

<table> 
<thead> <button> for each sorting option (Name, Status, Start time...)
<tr> expanded with something similar to the original row, like `expanded-job-row-8`

@keithjgrant
Copy link
Member Author

Are we going to cover these launched by and Project sorting options?

At this point we don't have plans for additional sorting options beyond the column headers, but it's worth revisiting at some point to bring them back in an enhancement (it would take a bit of UI we haven't mocked up or worked through yet)

If the user selects a running/pending and a successful job the delete button is enabled and trying to delete both:

image

It looks like this is the current behavior in devel as well. Can you file an issue and we can deal with it separately from this PR?

Are these data Started Time and Finish Time on the and expandable-row the same data or they are duplicate?

image

Yeah, these are duplicated from the non-expanded row. Though that's probably not necessary... Thoughts on this @trahman73 ?

Could you add a data-ouia-component-id (when available) or IDs on:

<table> 
<thead> <button> for each sorting option (Name, Status, Start time...)
<tr> expanded with something similar to the original row, like `expanded-job-row-8`

👍🏻

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

@tiagodread
Copy link
Contributor

Hey @keithjgrant look

Steps to reproduce:
1 click on the checkbox to select all rows

select all

On the checkbox row the aria-label is incorrect, "Select all rows":

image

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

tiagodread
tiagodread approved these changes Jan 27, 2021
View reviewed changes
Copy link
Contributor

@tiagodread tiagodread left a comment

Choose a reason for hiding this comment

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

Great job, added a PR to fix some broken tests and add more coverage about the list here https://github.com/ansible/tower-qa/pull/5909

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded (gate pipeline).

@softwarefactory-project-zuul softwarefactory-project-zuul bot merged commit a534a80 into ansible:devel Jan 27, 2021
@tiagodread tiagodread mentioned this pull request Jan 28, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tiagodread tiagodread tiagodread approved these changes

@jakemcdermott jakemcdermott jakemcdermott approved these changes

@nixocio nixocio nixocio approved these changes

Assignees

@tiagodread tiagodread

Labels
component:ui type:enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@keithjgrant @tiagodread @nixocio @jakemcdermott @ryanpetrello @unlikelyzero @awxbot