OCLOMRS-1047:List of concepts - Text search and filters don't work anymore #743

suruchee · 2021-09-14T04:54:48Z

JIRA TICKET NAME:

Summary:

Fixes the Clear Filter button functionality

…ymore

coveralls · 2021-09-14T05:58:06Z

Coverage increased (+0.01%) to 46.68% when pulling 67a8a23 on suruchee:OCLOMRS-1047 into 04efed8 on openmrs:master.

HerbertYiga · 2021-09-14T06:13:02Z

src/apps/concepts/pages/ViewConceptsPage.tsx

-        includeAdded: generalFilters.includes("Include Added Concepts")
-      });
-    }
+    retrieveConcepts({


thanks @suruchee have you also tested on a working instance of ocl

hadijahkyampeire

Thanks for the fix @suruchee , it is working well but I tried to keep most of what we had and only add the necessary dependencies to the useEffect and it still works, let's do that so that we don't break some other parts.

hadijahkyampeire · 2021-09-14T11:56:01Z

src/apps/concepts/pages/ViewConceptsPage.tsx

-  useEffect(() => {
-    setContainerUrl(url.replace("/concepts", ""));
-  }, [url]);
+  const containerUrl = url.replace("/concepts", "");


We might not need to remove this useEffect, it might cause some bugs

hadijahkyampeire · 2021-09-14T11:56:31Z

src/apps/concepts/pages/ViewConceptsPage.tsx

-      containerType === SOURCE_VERSION_CONTAINER
-        ? retrieveSource(containerUrl)
-        : retrieveDictionary(containerUrl);
+    // we don't make this reactive(only depend on the initial values), because the requirement


Also let's keep this if condition so that it only runs when there is a url

hadijahkyampeire · 2021-09-14T11:59:45Z

src/apps/concepts/pages/ViewConceptsPage.tsx

-  }, [retrieveConcepts, retrieveDictionary, retrieveSource, containerUrl]);
-
+  }, [
+    retrieveConcepts,


Why did you remove the retrieveDictionary, retrieveSource and containerUrl, let's keep them there, the only problem was that we were not including the initialQ, url, page, limit and there other params in the dependecies, with that only change it works as expected, so let's keep what was there and add url, page, limit, initialQ and others that way we won't affect some other work or have bugs.

Thanks, @hadijahkyampeire, I have updated with your suggestions.

hadijahkyampeire · 2021-09-14T12:25:17Z

Also, the ClearFilters button is not working, we should create a follow-up ticket to have that work again cc @suruchee

hadijahkyampeire

LGTM, thanks for the fix.

hadijahkyampeire · 2021-09-14T12:49:33Z

@suruchee this same work has been covered in @jwnasambu PR, you should rebase and maybe make this PR fix the clear filters bug.

suruchee · 2021-09-14T16:27:13Z

Hi @ibacher and @hadijahkyampeire, the clear all button works with the code changes but only works in double click, I also tried changing dependency in viewConceptsPage but did not work. Can you guys have a look?

ibacher · 2021-09-14T16:49:28Z

src/apps/concepts/components/FilterOptions.tsx


  const clearAllFilters = () => {
    setCheckedClasses([]);
    setCheckedDataTypes([]);
    setCheckedGeneral([]);
    setCheckedSources([]);
+    goTo(url);


I actually wonder about this... Right now the behaviour is that I click each filter and then click the "APPLY FILTERS" link to actually update the view. The logic behind the way the "CLEAR ALL" button currently work is that it's just clearing the filters as if you had cleared them manually. You still need to click "APPLY FILTERS" for the changes to make.

I think the question is: is the user experience expectation that clicking on the "CLEAR ALL" button removes all the filters and updates the table, or do we want to just clear all the filters and let the updating of the table take place after the user has reset their filters.

There are two reasons I might click on the "CLEAR ALL" button:

I really want to go back to the view of all concepts.

I just want to clear out the filters I previously had so I can create a new set of filters.

This new behaviour obviously makes the flow easy for 1, but is it the right flow for 2?

And in either case, I would update this PR to reflect what this change actually does (i.e. reloads the table when clicking "CLEAR ALL") rather than the now disconnected ticket it was first attached to.

I think the "CLEAR Filter" button should remove all the filters, update the url and make a request since the URL has changed so that the user goes back to the view without filters but we should remember to keep page and q since they are not attached to the other filters.

q makes sense to keep, but not page if we're making a new query (why would I want to be on page 5 of all the concepts after having been on page 5 of all the question concepts?)

That would be great @ibacher if you can update this PR with that fix, I will update the ticket description to capture the clear filter changes.

You are right @ibacher , now I see it for the page setting to 1, though given where the clear filters button is placed it looks like it's only for clearing the filters in the sideBar and not anything else. But I think that's a minor UX thing, now that I think of it, it's probably what the user expects.

Thanks, @hadijahkyampeire, and @ibacher I think we should leave it as it is for now. So, just closing the PR.

suruchee added 2 commits September 14, 2021 10:37

OCLOMRS-1047:List of concepts - Text search and filters don't work an…

90ea5f9

…ymore

fix for create custom concept icon

acd9f55

HerbertYiga reviewed Sep 14, 2021

View reviewed changes

hadijahkyampeire reviewed Sep 14, 2021

View reviewed changes

hadijahkyampeire requested a review from ibacher September 14, 2021 12:04

use effect and condition for url check added

b0ef122

hadijahkyampeire approved these changes Sep 14, 2021

View reviewed changes

suruchee added 2 commits September 14, 2021 19:02

rebase with master

96b59b5

fix for clear all not working

67a8a23

ibacher reviewed Sep 14, 2021

View reviewed changes

hadijahkyampeire added Do not merge yet Hold on to merge for some reasons help wanted Extra attention is needed labels Sep 14, 2021

suruchee closed this Sep 17, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OCLOMRS-1047:List of concepts - Text search and filters don't work anymore #743

OCLOMRS-1047:List of concepts - Text search and filters don't work anymore #743

suruchee commented Sep 14, 2021 •

edited by hadijahkyampeire

Loading

coveralls commented Sep 14, 2021 •

edited

Loading

HerbertYiga Sep 14, 2021

hadijahkyampeire left a comment

hadijahkyampeire Sep 14, 2021

hadijahkyampeire Sep 14, 2021

hadijahkyampeire Sep 14, 2021

suruchee Sep 14, 2021

hadijahkyampeire commented Sep 14, 2021

hadijahkyampeire left a comment

hadijahkyampeire commented Sep 14, 2021 •

edited

Loading

suruchee commented Sep 14, 2021

ibacher Sep 14, 2021

hadijahkyampeire Sep 14, 2021

ibacher Sep 14, 2021

hadijahkyampeire Sep 14, 2021

hadijahkyampeire Sep 14, 2021

suruchee Sep 17, 2021

OCLOMRS-1047:List of concepts - Text search and filters don't work anymore #743

OCLOMRS-1047:List of concepts - Text search and filters don't work anymore #743

Conversation

suruchee commented Sep 14, 2021 • edited by hadijahkyampeire Loading

JIRA TICKET NAME:

Summary:

coveralls commented Sep 14, 2021 • edited Loading

Choose a reason for hiding this comment

hadijahkyampeire left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hadijahkyampeire commented Sep 14, 2021

hadijahkyampeire left a comment

Choose a reason for hiding this comment

hadijahkyampeire commented Sep 14, 2021 • edited Loading

suruchee commented Sep 14, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

suruchee commented Sep 14, 2021 •

edited by hadijahkyampeire

Loading

coveralls commented Sep 14, 2021 •

edited

Loading

hadijahkyampeire commented Sep 14, 2021 •

edited

Loading