OCLOMRS-1044:Bug Fix: Pick Concepts from Source and Dictionaries #746

jwnasambu · 2021-09-22T19:57:52Z

JIRA TICKET NAME:

Bug Fix: Pick Concepts from Source and Dictionaries

Summary of what I worked on:

I added a toggle to toggle between sources and dictionaries and only show one at a time.
Introduced a search box, i.e., a blank text field that we can used to filter the shown sources and dictionaries
Ensured all the dictionaries/sources are pulled from the backed.
created a ConceptSourceSwitcher component

coveralls · 2021-09-22T20:14:58Z

Coverage decreased (-0.6%) to 45.838% when pulling 3b1ecf2 on jwnasambu:OCLOMRS-1044 into 0de0c31 on openmrs:master.

jwnasambu · 2021-09-22T20:23:33Z

This is a screenshot of what is implemented in the first part of this ticket.
https://user-images.githubusercontent.com/33891016/134416217-d542cec4-9f0c-4f1c-ae70-0edb801a6e77.mp4

ibacher · 2021-09-24T16:39:10Z

src/apps/concepts/components/ViewConceptsHeader.tsx

+           color="primary"
+              onClick={() => setShowSources(!showSources)}
+          > 
+             {showSources ? "Choose a source/dictionary" : "Select a different source/dictionary"}


Maybe this?

Suggested change

{showSources ? "Choose a source/dictionary" : "Select a different source/dictionary"}

{showSources ? "Select a recommended source" : "Select an alternative source"}

@ibacher thanks for the review.

ibacher

@jwnasambu Thanks for this initial step. A few small adjustments and this button should be good.

ibacher · 2021-09-28T13:33:46Z

src/apps/concepts/components/ViewConceptsHeader.tsx

+        <Button 
+           variant="contained" 
+           color="primary"
+              onClick={() => setShowSources(!showSources)}


Suggested change

onClick={() => setShowSources(!showSources)}

onClick={() => setShowSources(!showSources)}

ibacher · 2021-09-28T13:35:53Z

src/apps/concepts/components/ViewConceptsHeader.tsx

-          />
+        <Button 
+           variant="contained" 
+           color="primary"


This isn't a "primary" button... in fact, we probably don't really want any color for it, so the default is probably good enough.

Suggested change

color="primary"

ibacher · 2021-09-28T13:36:17Z

src/apps/concepts/components/ViewConceptsHeader.tsx

-            onClick={() => setShowSources(!showSources)}
-          />
+        <Button 
+           variant="contained" 


It would be better to use the text variant here so it keeps the same UX

Suggested change

variant="contained"

variant="text"

jwnasambu · 2021-09-29T14:05:15Z

@ibacher kindly I implemented the proposed changes above and I tried to introduce a search box, i.e., a blank text field that we can use to filter the shown sources and dictionaries but am stack and I kindly need your help! I want to use a stack which is from '@mui/material/Stack' am wondering if its same framework with materialUI and only differentiated by the tag.

ibacher · 2021-09-29T14:10:27Z

mui is the rebrand of Material UI for their 5.x release. The documentation for the version we use can be found here. There's no Stack in Material UI v4, but you can use a column-oriented Grid, e.g.,

<Grid
  container
  direction="column"
  justifyContent="center"
  alignItems="center"
>
</Grid>

jwnasambu · 2021-09-29T14:13:10Z

@ibacher thanks so so much! I really appreciate your help

jwnasambu · 2021-09-29T20:30:08Z

@ibacher with the current changes this is what I have

Kindly feel free to comment.

jwnasambu · 2021-09-30T05:54:45Z

@ibacher With the current changes this is the output kindly feel free to comment.

screencast.2021-09-30.8.AM-45-54.mp4

ibacher · 2021-09-30T16:30:47Z

src/apps/concepts/components/ViewConceptsHeader.tsx

+        {/* <Button 
+           variant="text" 
+           onClick={() => setShowSources(!showSources)}
+          > 
+             {showSources ? "Select a recommended source" : "Select an alternative source"}
+        </Button> */}
+        <Input
+          // onChange={e => setQ(e.target.value)}
+          // value={q}
+          color="secondary"
+          type="search"
+          fullWidth
+          placeholder={`choose a source`}
+          data-testid="searchInput"
+          endAdornment={
+            <InputAdornment position="end">
+              <IconButton
+                onClick={() => setShowSources(!showSources)}
+                data-testid="searchButton"
+              >
+                <SearchIcon />
+              </IconButton>
+            </InputAdornment>
+          }
+        />


@jwnasambu I can see where you're going here, but we really do want two independent elements that have relatively independent functions. Part of this is because it's very counter-intuitive (I think) to switch the list based on the icon in the search bar when that has... no obvious connection to the two lists displayed.

The onClick handler in the IconButton should trigger a search against the current list of sources (and we should only display the search bar when looking at the alternative sources, not the default sources.

I mentioned on Talk, I think, that we should use a Grid component to keep everything here orientated in a single column. So I would wrap both the Button and the Input in an element like this:

<Grid container direction="column"> // content </Grid>

@ibacher kindly ignore my current changes for I made without reading through your comment.

jwnasambu · 2021-10-04T16:05:55Z

@ibacher just to ensure I don't take a wrong direction kindly help me review this work please!

jwnasambu · 2021-10-05T12:38:16Z

@ibacher Am scared of diverting from the workflow you gave me. With the changes made this is what I have

screencast.2021-10-05.3.PM-24-37.mp4

ibacher · 2021-10-05T13:33:57Z

@jwnasambu We always want the button to switch between recommended and alternative sources to be visible (and the first thing in the list). It's only the Input field whose visibility should be toggled by which view we're on. So, rather than placing things in a big ternary (... ? ... : ...), we should have:

<Grid>
  <Button />
  {showSources && <Input />}
</Grid>

jwnasambu · 2021-10-05T14:57:33Z

@ibacher I desire to be with you on the same page. Kindly review my changes please!

jwnasambu · 2021-10-06T12:44:54Z

@ibacher kindly help me review this PR please!(thought still fixing the issue)

jwnasambu · 2021-10-06T21:37:03Z

@iabcher when I use this format

<Grid>     
  <Button />
  {showSources && <Input />      
</Grid>

I'm unable to see the search box, but when I remove the button this is what I have

screencast.2021-10-07.12.AM-23-28.mp4

jwnasambu · 2021-10-13T21:36:37Z

@ibacher this is the current output locally though the build is failing on the PR.

screencast.2021-10-14.12.AM-07-18.mp4

src/apps/concepts/components/ViewConceptsHeader.tsx

ibacher · 2021-10-18T18:55:01Z

src/apps/dictionaries/redux/actions.ts

@@ -381,7 +381,7 @@ export const recursivelyAddConceptsToDictionaryAction = (
      }

      if (!concept.hasOwnProperty("source_url") || !!!concept.source_url) {
-        concept.source_url = concept.url.split("/", 5).join("/") + "/";
+        return{...concept,source_url: concept.url.split("/", 5).join("/") + "/"}


Suggested change

return{...concept,source_url: concept.url.split("/", 5).join("/") + "/"}

return {...concept, source_url: concept.url.split("/", 5).join("/") + "/"};

ibacher · 2021-10-18T18:55:30Z

src/apps/concepts/pages/ViewConceptsPage.tsx

@@ -231,7 +232,8 @@ const ViewConceptsPage: React.FC<Props> = ({
        generalFilters: generalFilters,
        sourceFilters: sourceFilters,
        page: 1,
-        q
+        q,
+


Why the empty line here?

ibacher · 2021-10-18T18:55:38Z

src/apps/concepts/pages/ViewConceptsPage.tsx

+      dictionaries={dictionaries} 
+      showOnlyVerified={showOnlyVerified} 
+      toggleShowVerified={(e)=>setShowOnlyVerified(e.target.checked)} 
+


Why the empty line here?

ibacher · 2021-10-18T18:56:45Z

src/apps/concepts/components/ViewConceptsHeader.tsx

@@ -84,6 +97,27 @@ const ViewConceptsHeader: React.FC<Props> = ({
      } else setPreferredSources(defaultSources);
    }, [showSources, sources, dictionaries]);

+    const fetchMoreData = () =>{
+      setTimeout(()=>{
+        const previousSources =preferredSources?.filter(Boolean).concat(Array.from({length:10}));


Suggested change

const previousSources =preferredSources?.filter(Boolean).concat(Array.from({length:10}));

const previousSources = preferredSources?.filter(Boolean).concat(Array.from({length: 10}));

ibacher · 2021-10-18T18:58:17Z

src/apps/concepts/components/ViewConceptsHeader.tsx

+      setTimeout(()=>{
+        const previousSources =preferredSources?.filter(Boolean).concat(Array.from({length:10}));
+        setPreferredSources(previousSources)
+
+      },1000);


This looks like some sort of simulated code? I think before we get all of these elements looped in we need to start smaller, with just either the search or the toggle. It might be easiest, oddly to just start with a search box and show a default page of 10 results or w/e.

@ibacher I have just commended out the code for now!

jwnasambu · 2021-10-21T20:20:30Z

@ibacher kindly feel free to review what I have though still flopping.

ibacher · 2021-10-22T17:06:25Z

src/apps/concepts/components/ViewConceptsHeader.tsx

+            placeholder= {"Select an alternative source"}
+            value={q}
+            onChange = {e => setQ(e.target.value)}
+            data-testid="switch-source"


data-testid is just used to supply an id to identify something with via tests. It should only be used as a last resort. Since we don't need it here, we shouldn't have it. If we do need to assign a testid to this element, it must not be one that we've used elsewhere. This probably also applies to the other data-testid attributes in this PR.

Suggested change

data-testid="switch-source"

@ibacher I have removed all the data-testid="switch-source" that I added. Kindly feel free to review my PR again.

@jwnasambu Since there's a unit test looking for the data-testid="switch-source", we can leave it in, we just need to have it assigned to as single element as it was before.

src/apps/concepts/components/ViewConceptsHeader.tsx

ibacher · 2021-10-26T13:56:52Z

Cool! We've got all the existing tests passing. So, one thing: would you mind reformatting the code? You should be able to do this by running npx prettier --write src/apps/concepts/components/ViewConceptsHeader.tsx.

Then we need to work on getting the data properly.

So what we've been trying to do with this is change the menu to select sources so that when you are selecting from "all available" you have a list either filtered to sources or dictionaries. This allows us to provide (I hope) a better experience.

The first thing we need is an accurate count of the number of sources and / or dictionaries to fill into the InfiniteScroll component's dataLength field. To get this, the process is roughly the same as getting the number of counts in a source / dictionary. For this, @merovingienne's great work in this PR can serve as an example. Note that it basically involves reading the num_found header in the results.

So what we want to do is issue a query to the backend to get all the sources / dictionaries and use the num_found header to populate the dataLength field.

jwnasambu · 2021-10-26T16:58:56Z

@ibacher thanks so so much am going to do it after the call and share the experience. Thanks for the review!

merovingienne · 2021-10-27T14:17:51Z

@ibacher thank you for your kind words!

@jwnasambu let me know if you want to get anything clarified from PR 754.

jwnasambu · 2021-10-27T15:39:17Z

@merovingienne Thanks so much! I will surely do it. Am so humbled for your wiliness to help.

jwnasambu · 2021-12-08T17:13:35Z

@ibacher kindly help me review the changes made at your convenient time please!

jwnasambu marked this pull request as draft September 22, 2021 19:58

ibacher reviewed Sep 24, 2021

View reviewed changes

jwnasambu force-pushed the OCLOMRS-1044 branch from a7abcd0 to 41bf3d7 Compare September 26, 2021 10:46

ibacher requested changes Sep 28, 2021

View reviewed changes

ibacher requested changes Sep 30, 2021

View reviewed changes

jwnasambu force-pushed the OCLOMRS-1044 branch from ac109cb to ac397b8 Compare October 4, 2021 13:11

jwnasambu force-pushed the OCLOMRS-1044 branch 3 times, most recently from d59aba1 to a3aa7d6 Compare October 5, 2021 12:19

jwnasambu force-pushed the OCLOMRS-1044 branch 2 times, most recently from 5365eca to 50effd5 Compare October 6, 2021 12:43

jwnasambu force-pushed the OCLOMRS-1044 branch 5 times, most recently from bf1d1fd to d85ff7a Compare October 13, 2021 20:58

jwnasambu marked this pull request as ready for review October 14, 2021 15:56

jwnasambu force-pushed the OCLOMRS-1044 branch from 6a0bc2e to c93b4c6 Compare October 17, 2021 11:48

ibacher reviewed Oct 18, 2021

View reviewed changes

jwnasambu force-pushed the OCLOMRS-1044 branch from 36f3f37 to c616572 Compare October 18, 2021 22:14

ibacher reviewed Oct 22, 2021

View reviewed changes

ibacher reviewed Oct 25, 2021

View reviewed changes

src/apps/concepts/components/ViewConceptsHeader.tsx Outdated Show resolved Hide resolved

jwnasambu force-pushed the OCLOMRS-1044 branch 2 times, most recently from b15f9f8 to 9b34f06 Compare October 25, 2021 19:49

jwnasambu force-pushed the OCLOMRS-1044 branch from 835272c to c6fb970 Compare October 28, 2021 08:45

jwnasambu requested a review from ibacher October 28, 2021 17:30

jwnasambu force-pushed the OCLOMRS-1044 branch 2 times, most recently from 9f84dab to fe7a7bb Compare November 9, 2021 09:29

jwnasambu force-pushed the OCLOMRS-1044 branch from f417ea7 to 629adfd Compare December 1, 2021 10:37

jwnasambu force-pushed the OCLOMRS-1044 branch from d02faec to 7683e91 Compare December 8, 2021 19:53

jwnasambu force-pushed the OCLOMRS-1044 branch 3 times, most recently from a6ffa09 to f64d756 Compare January 18, 2022 09:55

hadijahkyampeire mentioned this pull request Jan 24, 2022

OCLOMRS-1016: Add support for selecting concepts from custom sources to the Bulk Import feature. #768

Open

OCLOMRS-1044:Bug Fix: Pick Concepts from Source and Dictionaries

3b1ecf2

jwnasambu force-pushed the OCLOMRS-1044 branch from cca44f6 to 3b1ecf2 Compare January 27, 2022 18:43

	{showSources ? "Choose a source/dictionary" : "Select a different source/dictionary"}
	{showSources ? "Select a recommended source" : "Select an alternative source"}

	onClick={() => setShowSources(!showSources)}
	onClick={() => setShowSources(!showSources)}

	return{...concept,source_url: concept.url.split("/", 5).join("/") + "/"}
	return {...concept, source_url: concept.url.split("/", 5).join("/") + "/"};

	const previousSources =preferredSources?.filter(Boolean).concat(Array.from({length:10}));
	const previousSources = preferredSources?.filter(Boolean).concat(Array.from({length: 10}));

OCLOMRS-1044:Bug Fix: Pick Concepts from Source and Dictionaries #746

Are you sure you want to change the base?

OCLOMRS-1044:Bug Fix: Pick Concepts from Source and Dictionaries #746

Conversation

jwnasambu commented Sep 22, 2021 • edited Loading

JIRA TICKET NAME:

Summary of what I worked on:

coveralls commented Sep 22, 2021 • edited Loading

jwnasambu commented Sep 22, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ibacher 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

jwnasambu commented Sep 29, 2021

ibacher commented Sep 29, 2021

jwnasambu commented Sep 29, 2021

jwnasambu commented Sep 29, 2021

jwnasambu commented Sep 30, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jwnasambu commented Oct 4, 2021 • edited Loading

jwnasambu commented Oct 5, 2021

ibacher commented Oct 5, 2021

jwnasambu commented Oct 5, 2021

jwnasambu commented Oct 6, 2021 • edited Loading

jwnasambu commented Oct 6, 2021 • edited Loading

jwnasambu commented Oct 13, 2021 • edited Loading

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

jwnasambu commented Oct 21, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ibacher commented Oct 26, 2021

jwnasambu commented Oct 26, 2021

merovingienne commented Oct 27, 2021 • edited Loading

jwnasambu commented Oct 27, 2021

jwnasambu commented Dec 8, 2021

jwnasambu commented Sep 22, 2021 •

edited

Loading

coveralls commented Sep 22, 2021 •

edited

Loading

jwnasambu commented Oct 4, 2021 •

edited

Loading

jwnasambu commented Oct 6, 2021 •

edited

Loading

jwnasambu commented Oct 6, 2021 •

edited

Loading

jwnasambu commented Oct 13, 2021 •

edited

Loading

merovingienne commented Oct 27, 2021 •

edited

Loading