(feat) O3-2790: Create a reusable location picker component

[jayasanka-sack]

Requirements

• This PR has a title that briefly describes the work done including the ticket number. Ensure your PR title includes a conventional commit label (such as feat, fix, or chore, among others). See existing PR titles for inspiration.

For changes to apps

• My work conforms to the O3 Styleguide and design documentation.

If applicable

• My work includes tests or is validated by existing tests.
• I have updated the esm-framework mock to reflect any API changes I have made.

Summary

This PR introduces a significant enhancement by establishing a reusable location picker through the relocation of the current picker to our styleguide. The key modifications include:

• Component Refactoring:
  • Extracted the existing location picker from the login process.
  • Developed a new, standalone component that functions as a versatile location picker.
• Bug fixes:
  • Fix https://openmrs.atlassian.net/browse/O3-2759
• Other:
  • Designed the location picker to accommodate a default location UUID. Ensured that the default location appears at the top of the list, even if it falls outside the initial set of loaded locations.
  • Updated the component to not to show the default location if the search results doesn't contain the default location
  • Write and update existing test cases

Screenshots

No visual changes

Related Issue

https://openmrs.atlassian.net/browse/O3-2790

Other

[jayasanka-sack]

@ibacher The e2e test is failing for this PR as we don't package esm-framework. What can we do?

https://github.com/openmrs/openmrs-esm-core/blob/main/e2e/support/github/run-e2e-docker-env.sh

[ibacher]

Several things:

• What connections does this PR (move the location picker to the styleguide) have with the ticket (additional locations not loading in Chrome-based browsers)?
• I don't think data-fetching components, in general, belong in the styleguide without a strong justification. I'm having trouble seeing where else this component is useful.
• In general, we do not want to end up with a large component library because that becomes a maintenance headache.

[ibacher]

See previous comment as well.

[ibacher]

I don't think .module.scss is consistent with our naming conventions (and the couple of other places it's used should be fixed).

[jayasanka-sack]

I tried using without .module suffix earlier and the css didn't get modularised (#910 (comment)). Then I thought that it was the intended behaviour as the files doesn't have the suffix used to write normal css classes. ex: Snackbar

[brandones]

Yeah @ibacher unfortunately it's a webpack issue, .module.scss magically makes the styles work. I don't understand it. @denniskigen pointed it out to me.

[jayasanka-sack]

Thanks for your thoughtful review, @ibacher. Let me try address your queries here:

• The main ticket for this PR is O3-2790. It fixes O3-2759 as well.
• The idea for moving it to the style guide came from this this bug O3-2610. We came to this conclusion on the O3 lead call 2 weeks ago.
• The location picker is used on
  • Login
  • Programs
  • Start / edit visit
  • Form Engine
  • Appoinments
  • Cohort builder
• Each of these places has it's own location picker implementation. Apart from the login, all other places built with the false assumption that location API load all the locations at once. Some of them (ex: programs, appointment) has bad UX and produce some unexpected results like O3-2610.

I appreciate your feedback and would love to hear your thoughts. :)

[ibacher]

@jayasanka-sack So, I'm on-board with a component for viewing locations. I'm less keen on the data-fetching part living in the styleguide. That is, I think the fetching should be handled by a component that mounts this component and passed in as props rather than having the fetch logic itself in the styleguide. There might be a case for putting those hooks in esm-react-utils...

There are issues with components in the styleguide, like the translation system won't work nor will the configuration system. These need to come via props to the component. Components in the styleguide should really just be, effectively the HTML markup and component behaviour (like drop-downs or something).

[ibacher]

And, yeah, for the e2e tests in core, we also need to run the build step.

[brandones]

Yeah let's please file a ticket specifically about this, under https://openmrs.atlassian.net/browse/O3-1422. Do O3-2790 separately. Whoops just saw that you changed the ticket, sorry I got them mixed up! I've added O3-2790 to the styleguide components epic.

Please also document the places in the code that this location picker would be used. What existing location pickers exist throughout O3? It's important to make sure that styleguide components are very well designed in terms of being adequate for current use cases (while being minimalist in their API and simple in their implementation).

I don't have strong feelings about whether the data fetching is included or not. I'm inclined to say that whether it should be included should depend on whether users of the component will want control over the data that gets used. For example, the Patient Banner Contact Details component fetches the patient object; this is fine because it would be very strange for someone to want to customize the patient data being used. But here, there is already the location tag prop, which indicates some need for customization. On the other hand, Infinite Scroll requires a pretty high degree of coupling between the UI and data fetcher. So it might be hard to design a good component API where the data is provided by the component user.

[brandones]

We do have esm-styleguide configuration—importantly, if the location picker uses that, it will be the same across the app.

And we now support core translations, as you all know :)

[brandones]

@jayasanka-sack I have merged this with the latest. Please give it a look over to see if you think I messed anything up. It was an extremely messy merge.

[brandones]

Also, the build is failing. Unfortunately it is a pretty difficult error to debug. @jayasanka-sack will you have time to work on this and get this in?

Once the build is passing I would support merging this in, if @ibacher agrees. I like the approach you've taken and think you did a great job with it. Unless there are reasons we might want the function-user to be able to customize the list of locations going in to the picker, I think having the picker handle the fetching/paging/etc itself is the right call.

[jayasanka-sack]

Thanks @brandones ! Let me try to fix this.