Feature #17579

Clear table filter when changing the project

Added by Daniel Kutyła 9 months ago. Updated about 1 month ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
Workbench2
Target version:
Start date:
12/02/2021
Due date:
% Done:

100%

Estimated time:
(Total: 0.00 h)
Story points:
-

Description

Common usage pattern:

User is filtering the content of a project for subprojects by a specific search pattern

User is selecting the found subproject to see what is inside
The project is appears empty because the filter entered before is still set and not cleared when changing the route

🚨 PANIC🚨
Desired behaviour: The "Search" field should be cleared when changing to a different route. This should also be done for other filters in the table as, imho, switching the project switches the context and any applied filter makes no sense anymore


Subtasks

Task #18546: Review 17579-Clear-table-filter-when-changing-the-projectResolvedDaniel Kutyła

Associated revisions

Revision fc84a3f3
Added by Daniel Kutyła about 1 month ago

Merge branch '17579-Clear-table-filter-when-changing-the-project' into main
closes #17579

Arvados-DCO-1.1-Signed-off-by: Daniel Kutyła <>

History

#1 Updated by Daniel Kutyła about 2 months ago

  • Target version set to 2021-12-08 sprint
  • Assigned To set to Daniel Kutyła
  • Status changed from New to In Progress

#2 Updated by Daniel Kutyła about 2 months ago

  • Priority changed from Normal to Urgent

#3 Updated by Daniel Kutyła about 2 months ago

New version first commit: https://dev.arvados.org/projects/arvados-workbench-2/repository/revisions/9ec32d73b78aeb92b842aa8cf08bc859e084dcb1
Test run: https://ci.arvados.org/job/developer-tests-workbench2/525
Branch: 17579-Clear-table-filter-when-changing-the-project

Search input field should now be cleared

#4 Updated by Lucas Di Pentima about 2 months ago

Some comments:

  • There were a couple of failed tests on Jenkins.
  • At the new cypress test, I believe you can avoid creating the second project, as it isn't being used. Could you also add a test for the collection file browser?
  • I think it would be convenient to add a unit test with this new feature on src/components/search-input/search-input.test.tsx. With the cypress tests we're confirming that the SearchInput users are correctly passing the new prop to it, but a test for the new feature itself would be useful too, wdyt?
  • Is the file src/components/collection-panel-files/collection-panel-files2.tsx the one holding the old file browser? I don't believe we should keep it as a duplicate, adding maintenance burden without getting any benefit from it. If we need to get it back for some reason (which I seriously doubt), we could just use git's features to bring it back from the past, do you agree?
  • At file src/components/data-explorer/data-explorer.tsx Line 89: Does it really need to be added in the local state? It seems that later on it isn't retrieved from it, or am I missing something? (as a quick test I've just commented those lines and the search input clearing still works)

#5 Updated by Daniel Kutyła about 2 months ago

New version first commit: https://dev.arvados.org/projects/arvados-workbench-2/repository/revisions/aac3dfbed3d02cc6a2240add62439ee0dda3f335
Test run: https://ci.arvados.org/job/developer-tests-workbench2/530
Branch: 17579-Clear-table-filter-when-changing-the-project

Fixed tests, cleaned file structure

#6 Updated by Peter Amstutz about 2 months ago

  • Target version changed from 2021-12-08 sprint to 2022-01-05 sprint

#8 Updated by Lucas Di Pentima about 1 month ago

The updates look good, but I have a couple of questions regarding the SearchInput component unit test:

  • At file src/components/search-input/search-input.test.tsx:
    • Line 99: I think it would be convenient to also add an opposite case so that it's really clear what the behavior of the component should be with and without the selfClearProp prop, do you agree?
    • By reading the new test, it's not 100% clear to me what's trying to prove, for example if I change the setProps call at line 101 to pass an 'abc' argument instead of 'aaa', the test still succeeds, but OTOH if I comment out line 101 entirely, the test fails. I think both cases should be equivalent so maybe the test should be more complete or the code being tested doesn't behave as expected?

#9 Updated by Daniel Kutyła about 1 month ago

Everything work as expected the selfClearProp is a trigger for the value clear and it should point to the unique element of the component that is using for example for the project it will be project uuid, so when we change the project the search input will clear itself as selfClearProp will change

#10 Updated by Daniel Kutyła about 1 month ago

New version first commit: https://dev.arvados.org/projects/arvados-workbench-2/repository/revisions/17dfef9d2440ddbac9cd8eac0ec8be721b42cc09
Test run: https://ci.arvados.org/job/developer-tests-workbench2/539
Branch: 17579-Clear-table-filter-when-changing-the-project

I added better description for the test

#11 Updated by Lucas Di Pentima about 1 month ago

I don't think the better naming of the test addresses neither one of my comments, but if you believe they don't apply, please go ahead and merge.

#12 Updated by Daniel Kutyła about 1 month ago

  • % Done changed from 0 to 100
  • Status changed from In Progress to Resolved

Also available in: Atom PDF