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

Filter and sort functionality for Data Management table #750

Merged
merged 54 commits into from
Aug 28, 2023

Conversation

stalgiag
Copy link
Contributor

@stalgiag stalgiag commented Aug 21, 2023

Overview

This PR adds support for sorting and filtering Test Plans in the Data Management table as outlined in #678 and #648. The design of the UI follows the mockups provided in this comment.

Notes

  • The sorting and filtering logic was broken out into custom hooks to avoid dramatically inflating the size of the DataManagement component.
  • The APG sortable table example was followed for the creation of the SortableTableHeader component. When testing with VoiceOver for macOS, I noticed that there is no dynamic announcement when thearia-sort attribute of the currently focused header is changed. I believe this is an issue with VoiceOver as the output is identical on the APG example.
  • All sorting does primary active sorting first and then alphabetical by title secondarily. The sorting by AT is currently a stub that ends up doubling the behavior of the title sorting. I believe this can be changed once other work lands and test plans do not all have an identical list of ATs.
  • This PR relies heavily on string comparison. To avoid problems created by string typos, I added a constants.js file with enums. I did not see this pattern anywhere else in the codebase but in my experience it helps avoid bugs that are difficult to diagnose with string comparison. That said, the source of truth for some of those constants, especially TEST_PLAN_VERSION_PHASES, should likely be on the server once the dust has settled and other work has been merged. Let me know if this pattern is unwelcome or previously decided against, I would be happy to change it.

@howard-e
Copy link
Contributor

@stalgiag this looks super good! I've been super eager to see filters and sorting being included since the initial days of the Test Queue page so thank you! 🙂

I took an initial pass at it, functionality wise and noticed a few things that may need to be addressed first:

Screenshot showing filter buttons and top of Data Management Page; additional details follow this image

In the above screenshot, there are 2 things to note:

  • The values being calculated for the filter buttons may need to be checked again? It notes that there are 34 R&D Complete rows but just 1 In Candidate Review row. The screenshot shows I have at least 2 Candidate rows and my database instance during this test has 10 Candidate Test Plan Versions overall.

  • This is a fresh load of the page, but it seems the default sort order hasn't been maintained according to:

    Default Table Sort (from Design Missing Test Plan Status Management Functions #648)

    • First sort by overall status descending: recommended plans, then candidate plans, then draft plans, then R&D complete plans.
    • Then sort by test plan name ascending.
    • Then sort by Covered AT ascending
    • Shown in the screenshot is that the initial table rows are:
      • Link Example 1 ... in Candidate, then
      • Action Menu Button Using aria-activedescendant in Candidate, then
      • not in the screenshot is Action Menu Button Example Using element.focus() in R&D, then
      • Alert Example in Candidate
      • ... and so on with mixing of the R&D Candidate rows
    • It does seem the alphabetical order is maintained outside of the first row though
Screenshot shows Recommended Plans filter option being selected when there are 0 known results. With no data to show, the column titles of the first 4 columns intersect

The above screenshot shows the Recommended Plans filter option being selected but there are 0 known results. With no data to show, the column titles of the first 4 columns are intersecting. This could be a consequence of the css doing anything along the lines of using fit-content or auto width or some variation of having the cell's content set the width, with 1 or more of those columns.

I do think a better experience here could be to show a message that there is no data to show, rather than rendering an empty table though.

@stalgiag
Copy link
Contributor Author

stalgiag commented Aug 21, 2023

Thanks for checking it out! Looks like I missed most of these issues because of the limited variety present in my local DB which is usually just the blank reseeded default. This likely won't be a problem once I am able to use the migrated production DB but in the meantime I might go ahead and make myself a few different test DBs so I can catch problems like these sooner. Thanks for sending the copy of your DB @howard-e !

  • The values being calculated for the filter buttons may need to be checked again? It notes that there are 34 R&D Complete rows but just 1 In Candidate Review row. The screenshot shows I have at least 2 Candidate rows and my database instance during this test has 10 Candidate Test Plan Versions overall.

Interesting. Will modify my DB to reproduce.

This is a fresh load of the page, but it seems the default sort order hasn't been maintained according to ...

The code follows the sort order so I think that is a bug specific to that first Test Plan Version. I'll check it out.

The above screenshot shows the Recommended Plans filter option being selected but there are 0 known results. With no data to show, the column titles of the first 4 columns are intersecting. This could be a consequence of the css doing anything along the lines of using fit-content or auto width or some variation of having the cell's content set the width, with 1 or more of those columns.

I do think a better experience here could be to show a message that there is no data to show, rather than rendering an empty table though.

I agree. Perhaps we should even take it a step further and not show the filter button for a phase with out any test plan versions.

Edit: With the new DB, the issue became apparent. My code doesn't account for multiple test plan versions for a single test plan having different phases. Will fix first thing in the morning!

…ns with multiple versions, ...

useDataManagementTableFiltering don't generate label for buttons that have no associated plans
@stalgiag
Copy link
Contributor Author

stalgiag commented Aug 22, 2023

Okay updated the functionality as per our discussion @howard-e . I broke out a lot of logic for sorting by phase and deriving overall phase for a test plan into custom hooks. This could open up some opportunities for refactoring in DataManagementRow but I will leave that code as-is for now.

Note that "All plans" will have a larger number than the sum of the remaining buttons if there are plans that haven't been started yet.

I still want to write new unit tests for the additional hooks and to ensure that instances with multiple versions per test plans are handled correctly but this can be reviewed at any time.

… multiple test plan versions for a single test plan

hook rename
Copy link
Contributor

@alflennik alflennik left a comment

Choose a reason for hiding this comment

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

Damn, nice filtering and sorting!

I went through all the code and I don't have much to comment on, beyond some very minor points below. Here are a couple observations I had from testing it out:

  • Minor point: the spacing seems to be greater between the filter buttons than I would expect, I like the spacing between the "Filter" text and the first button, can that spacing be repeated for the following buttons?
  • I'm struggling to articulate this, but when I click on the sort buttons it seems like sometimes they go from asc to desc and sometimes they don't. Would it be possible for the first click to activate the sorting for that column without switching from asc to desc and then the second click switch from asc to desc? If that doesn't make sense I can try to articulate it more clearly and possibly record a video of my screen.

client/components/DataManagement/hooks.js Outdated Show resolved Hide resolved
client/utils/constants.js Outdated Show resolved Hide resolved
client/components/DataManagement/hooks.js Outdated Show resolved Hide resolved
…ns with multiple versions, ...

useDataManagementTableFiltering don't generate label for buttons that have no associated plans
… multiple test plan versions for a single test plan

hook rename
@stalgiag
Copy link
Contributor Author

What do you think about using a (probably hidden) aria-live element instead and updating the content of that when the sort happens?

@howard-e I have taken this approach. I made a generic AriaLiveRegionProvider that can wrap a set of elements and provide access to a hidden live region that can be updated with a polite announcement. Now changes are announced in a similar way to the example you shared with me. I was only able to test on VoiceOver. It is a little verbose but very clear what is happening.

@howard-e
Copy link
Contributor

What do you think about using a (probably hidden) aria-live element instead and updating the content of that when the sort happens?

@howard-e I have taken this approach. I made a generic AriaLiveRegionProvider that can wrap a set of elements and provide access to a hidden live region that can be updated with a polite announcement

Nice!

Now changes are announced in a similar way to the example you shared with me. I was only able to test on VoiceOver. It is a little verbose but very clear what is happening.

I think I prefer it this way. "Test Plans Status Summary Table, now sorted by Test Plans in desecending order" sounds better to me than just clicking it and hearing "sorted by descending", so it feels fine to me! But we'll see what others think and any changes there should be easy enough to make with how AriaLiveRegionProvider works. Thanks!

@stalgiag stalgiag changed the base branch from update-database-impl to main August 28, 2023 19:38
@stalgiag stalgiag changed the base branch from main to update-database-impl August 28, 2023 19:38
@howard-e howard-e merged commit 3357740 into update-database-impl Aug 28, 2023
@howard-e howard-e deleted the filtering-sort-data-management branch August 28, 2023 19:51
howard-e added a commit that referenced this pull request Sep 28, 2023
…abase Implementation to support #648 (#688)

Changes added to primarily support #648, but also #518 and w3c/aria-at#950. This includes functional changes to the Data Management, the Test Queue and the Candidate Review pages. It also includes changes to the database structure, described in #632.

* feature: updates functionality of Data Management page (#713)
* feature: updates functionality of Test Queue page (#715)
* feature: updates functionality of Candidate Review page (#715) 
* feature: include functionality to support the concept of required reports (#722)
* feature: adds Test Plan Report Status dialog (#728)
* enhancement: move Candidate Phase Start Date and Target Completion Date into dedicated columns on Candidate Review page (#730)
* feature: adds Test Plan Version page (#747)
* enhancement: explicitly support ‘DEPRECATED’ phase status for TestPlanVersions (#749)
* feature: adds filter and sorting functionality by columns headers on Data Management page (#750)
* bugfix: update semantic structure of cells with multiple list items on Data Management page (#752)
* enhancement: include GitHub issues on Test Plan Version page (#753)
* bugfix: revision of the required reports conditions for updating to CANDIDATE and RECOMMENDED phases (#764)
* bugfix: removes superfluous header from Test Plan Report Status dialog (#766)
* bugfix: update and revise sorts, headings and descriptions of elements on Test Plan Version page (#767)
* bugfix: account for several other update phase scenarios that could prevent the update from happening if there is an older TestPlanVersion that exists with results (#771)
* bugfix: update headings and revise deprecated dates shown on Test Plan Version page (#773)
* enhancement: allow updating of GitHub issues being presented in the app to be more easily understood (#775)
* bugfix: correct deprecatedAt date to be relative to when the ‘next’ TestPlanVersion was added (#780)
* enhancement: update the text shown when deprecation occurs during a phase on Test Plan Version page (#781)
* bugfix: fix inverted sort descriptions and pin sort of of Test Plan name columns on Data Management page (#790)

---------

Co-authored-by: Erika Miguel <[email protected]>
Co-authored-by: Paul Clue <[email protected]>
Co-authored-by: alflennik <[email protected]>
Co-authored-by: Stalgia Grigg <[email protected]>
Co-authored-by: Howard Edwards <[email protected]>
alflennik added a commit that referenced this pull request Oct 18, 2023
* Fix bug that would allow testPlanVersion to be updated to RECOMMENDED before the associated reports were all marked as final (but the tests were 100% done in the Test Queue)

* Refine raise an issue behavior (#753)

* Refine raise an issue behavior

* Address feedback

* Fixed squished dot icon

* Address last feedback

* Hide closed issues on datamgmt page

* Fix for test results not being automatically saved when navigating through Test Run

* Filter and sort functionality for Data Management table (#750)

* First pass on functioning sort buttons for DataManagePage

* Functioning sort

* Filter functionality for DataManagement

* Refactor filter buttons on DataManagement to reduce complexity

* Filter buttons as separate component

* DataManagement page, break sort out into dedicated hook

* DataManagement page, dedicated hook for filtering

* DataManagement, add constant for test plan version phases to simplify hooks code

* Add relevant dynamic aria attributes to DataManagement column sort elements

* Add relevant dynamic aria attributes to DataManagement filter buttons

* unit tests for SortableTableHeader

* unit tests for FilterButtons

* unit tests for useDataManagementTableSorting hook

* Add unit tests for useDataManagementTableFiltering hook

* Filter buttons only show buttons that have have label, handle testPlans with multiple versions, ...

useDataManagementTableFiltering don't generate label for buttons that have no associated plans

* Break out overall phase derivation logic into dedicated hooks, functional filter and sort

* Simplify useDerivedTestPlanOverallPhase

* Additional unit tests for additional hooks and to test scenarios with multiple test plan versions for a single test plan

hook rename

* Different UX click interaction sequence with SortableTableHeader

* Correct interpretation of alphabetical ascending/descending

* Rename DataManagement/hooks.js to filterSortHooks

* Move sorting and filtering enums to more specific locations based on use

* Fix file locations in unit tests, DataManagement, FilterButtons, SortableTableHeader

* Smaller margin between buttons, FilterButtons

* First pass on functioning sort buttons for DataManagePage

* Functioning sort

* Filter functionality for DataManagement

* Refactor filter buttons on DataManagement to reduce complexity

* Filter buttons as separate component

* DataManagement page, break sort out into dedicated hook

* DataManagement page, dedicated hook for filtering

* DataManagement, add constant for test plan version phases to simplify hooks code

* Add relevant dynamic aria attributes to DataManagement column sort elements

* Add relevant dynamic aria attributes to DataManagement filter buttons

* unit tests for SortableTableHeader

* unit tests for FilterButtons

* unit tests for useDataManagementTableSorting hook

* Add unit tests for useDataManagementTableFiltering hook

* Filter buttons only show buttons that have have label, handle testPlans with multiple versions, ...

useDataManagementTableFiltering don't generate label for buttons that have no associated plans

* Break out overall phase derivation logic into dedicated hooks, functional filter and sort

* Simplify useDerivedTestPlanOverallPhase

* Additional unit tests for additional hooks and to test scenarios with multiple test plan versions for a single test plan

hook rename

* Different UX click interaction sequence with SortableTableHeader

* Correct interpretation of alphabetical ascending/descending

* Rename DataManagement/hooks.js to filterSortHooks

* Move sorting and filtering enums to more specific locations based on use

* Fix file locations in unit tests, DataManagement, FilterButtons, SortableTableHeader

* Smaller margin between buttons, FilterButtons

* Cleanup after rebase

* Render SortableTableHeader inside <table> for test to dismiss warning

* Add AriaLiveRegionProvider, Update DataManagement table to use it

* Allow SortableTableHeader component to work without AriaLiveRegionProvider

* Explicitly support 'DEPRECATED' phase for `TestPlanVersion.phase` (#749)

* Start to support for sunset phase

* Explicitly use 'DEPRECATED' phase for TestPlanVersion.phase

* Add checks to prevent test plan versions in R&D or Deprecated from being shown in data management dropdown

* Remove duplicate updateTestPlanVersion call

* Reuse phase variable

* Add copy for deprecated reports with DisclaimerInfo component

* Exclude 'DEPRECATED' testPlanVersions on DataManagement page query

* Ensure only reports marked as final are displayed on /embed/reports/<pattern>

* Fix test

* Update failing test

* Adjust semantic structure on Data Management Page (#752)

* Adjust cell items for data management row to use list-related roles; update aria-labels

* Address PR feedback

* Remove width:max-content

* Address PR feedback

* Formatting

* Address feedback

* Adjust BasicModal to support AtAndBrowserDetailsModal closing

* Stop assign menu dropdown from creating unintended bottom space with the parent container

* Formatting

* Close #755

* Close #754

* Remove superfluous header from TestPlanReportStatusDialog (#766)

* Revise required reports conditions (#764)

* Revise required reports approach

* Update tests

* Revert dev.env

* Update Version History Page (#767)

* Apply correct sort order for Timeline for All Versions section

* Update headings used on Versions page

* Add aria-labelledby's for tables

* Add &nbsp; for applicable spaces so the text is properly announced by NVDA

* Add migration to add missing deprecatedAt dates and to also properly set the candidatePhaseReachedAt dates to more practical dates after the migrations (if candidatePhaseReachedAt < draftPhaseReachedAt, set it candidatePhaseReachedAt to draftPhaseReachedAt + 1day)

* Test Plan Versions Page: Use standard testPlanVersions descending sort and show all phases being included in Version Summary

* Switch issues search support for checking against hidden body content instead

* Fix TestPlanReportStatusDialog using d (day of the week) and y (era)

* Explicit check for older date when deprecating existing RD test plan versions during import

* Update hidden message content in github issue

* Update query for anon user on TestRun page

* Keep overall status pill from drifting to center of cell

* Fix R&D only TestPlanVersion's table not being shown

* Use aria-label for heading to prevent space being announced

* Update date format used in aria-label

* Make version history a drop down

* comment out unused variable

* Fix responsiveness

* Remove comments and logs

* Changes after merge

* Fix icon color

* Fix padding for timeline heading

* Fix console errors and delete comments

* Fix conflicts that caused error

* Implement required changes from review

---------

Co-authored-by: Howard Edwards <[email protected]>
Co-authored-by: Alexander Flenniken <[email protected]>
Co-authored-by: Stalgia Grigg <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants