(feat) O3-1831: Registration: Support person attribute of type Location #1032

usamaidrsk · 2024-03-13T21:10:15Z

Requirements

This PR has a title that briefly describes the work done including the ticket number. If there is a ticket, make sure your PR title includes a conventional commit label. See existing PR titles for inspiration.
My work conforms to the OpenMRS 3.0 Styleguide and design documentation.
My work includes tests or is validated by existing tests.

Summary

This PR add support for person attribute type Location, on patient registration.

test config

{
  "@openmrs/esm-patient-registration-app": {
    "fieldDefinitions": [
      {
        "id": "Health Center",
        "label": "Heath Center",
        "type": "person attribute",
        "uuid": "8d87236c-c2cc-11de-8d13-0010c6dffd0f"
      }
    ],
    "sections": [
      "demographics",
      "contact",
      "relationships",
      "extraInfo"
    ],
    "sectionDefinitions": [
      {
        "id": "extraInfo",
        "name": "Extra info",
        "fields": [
          "Health Center"
        ]
      }
    ]
  }
}

Screenshots

register-location.mp4

Related Issue

O3-1831

Other

brandones · 2024-06-21T14:03:24Z

@usamaidrsk Seems like this is very close. Can you wrap it up and I'll review?

brandones · 2024-07-09T16:18:15Z

Ping @usamaidrsk ? It's still failing CI and still a draft.

usamaidrsk · 2024-07-10T06:09:15Z

This work is being blocked by TRUNK-6246
cc @brandones cc @ibacher

brandones · 2024-07-21T17:27:02Z

@usamaidrsk Looks like the blocking issue has not been resolved, this is still in draft, and CI is failing. Why did you review request me and Ian?

packages/esm-patient-registration-app/src/config-schema.ts

...c/patient-registration/field/person-attributes/location-person-attribute-field.component.tsx

...rc/patient-registration/field/person-attributes/location-person-attribute-field.resource.tsx

brandones · 2024-08-19T16:01:16Z

Ping @usamaidrsk , it's been a month...

brandones · 2024-09-15T11:41:19Z

Ping again, @usamaidrsk

usamaidrsk · 2024-09-18T12:48:14Z

Ping again, @usamaidrsk

Hi @brandones
I am following up on this PR to be merged.
Thanks cc @mherman22 @ibacher

usamaidrsk · 2024-09-27T16:16:55Z

Hello @brandones, this PR is now ready for review
Thanks

packages/esm-patient-registration-app/src/config-schema.ts

...on-app/src/patient-registration/field/person-attributes/person-attribute-field.component.tsx

...c/patient-registration/field/person-attributes/location-person-attribute-field.component.tsx

brandones · 2024-10-01T17:44:24Z

...c/patient-registration/field/person-attributes/location-person-attribute-field.component.tsx

+        if (locationOptions.find(({ label }) => label === value)) return;
+        // If the input is a new value, set the search query
+        setSearchQuery(value);
+        // Clear the current selected value since the input doesn't match any existing options
+        setValue(null);


I would feel more confident about this if you'd posted a video showing you testing a few cases—starting to type a valid location name, typing nonsense, typing a location name exactly, selecting a location, changing the input text after having selected a location. I think this code makes sense but I don't have 100% confidence in it.

brandones · 2024-10-01T17:46:30Z

...rc/patient-registration/field/person-attributes/location-person-attribute-field.resource.tsx

+    urlSearchParameters.append('_summary', 'data');
+
+    if (!debouncedSearchQuery) {
+      urlSearchParameters.append('_count', '5');


The screenshot you posted shows more than 5 locations, with no search query. I guess the screenshot is out of date? Please update it. Ideally with a video.

Also, why such a small number?

I have added the video on the description, and for the count being 5, I was trying to eliminate the scroll on the select dropdown, but I can change that to a number you suggest.
Thanks

brandones · 2024-10-10T17:09:23Z

Thanks @usamaidrsk . The code looks much better, and the video is helpful. Unfortunately the UX seems quite bad, based on your video. You type "out" and "Outpatient clinic" disappears, you see all options disappear, and then nothing comes up, etc.

As a starting point for improving it, I think the list of options should not be cleared until results from the search have been returned by the server. It might also be nice to put a loading spinner on the right side of the input box while new results are loading, to indicate that the user's input to the search input box is being responded to.

usamaidrsk · 2024-10-16T13:42:57Z

Hi @brandones please take a look at this.
Thanks
cc @vasharma05
https://github.com/user-attachments/assets/4b9e6ff0-ef00-41c1-b754-ab981752e1fa

brandones · 2024-10-17T17:57:56Z

Much better, thanks @usamaidrsk . But having the loading indicator push the dropdown downward creates layout shift, which is a bad UX. Please adjust the loading indicator so that it doesn't create layout shift. Maybe it could float with absolute positioning toward the right side of the input box or something.

usamaidrsk · 2024-10-17T19:08:47Z

Thanks again @brandones for your feedback on the review.

Please adjust the loading indicator so that it doesn't create layout shift. Maybe it could float with absolute positioning toward the right side of the input box or something.

Regarding the loading indicator, at first I had it as you're suggesting. On desktop, I positioned the indicator on the right side of the input field. However, for tablet view, where the input spans full width, placing the indicator on the right required shrinking the input width while loading and then expanding it back once loading is complete, which seemed not good UX too.

I wanted to let you know about that before I do as suggested.
I would like to hear your thoughts on this again.

Thanks

ibacher · 2024-10-17T19:54:44Z

@usamaidrsk We should be using a Dropdown here which has a skeleton state to indicate that it's loading.

usamaidrsk · 2024-10-17T20:04:08Z

@usamaidrsk We should be using a Dropdown here which has a skeleton state to indicate that it's loading.

Thanks @ibacher
I believe this might be a slightly different use case. In this scenario, the loading indicator is shown while the user is actively searching/typing. Replacing the entire input with a a skeleton loader could potentially be confusing in this context.

cc @brandones

ibacher · 2024-10-17T20:07:19Z

I see. I'd probably opt for a loading indicator inside the combo box on its right edge.

usamaidrsk · 2024-10-17T20:11:36Z

I see. I'd probably opt for a loading indicator inside the combo box on its right edge.

Yeah, that sounds good. Could we have done something similar, that I can refer too.
Thanks

brandones · 2024-10-21T16:59:13Z

@usamaidrsk We haven't, but it's just CSS, should be pretty straightforward. Adding something to the right side of an input box (e.g. an eye icon, an x icon) is a very common thing to do, I'm sure there's plenty of info on Google/SO/etc.

usamaidrsk · 2024-10-22T16:58:09Z

@usamaidrsk We haven't, but it's just CSS, should be pretty straightforward. Adding something to the right side of an input box (e.g. an eye icon, an x icon) is a very common thing to do, I'm sure there's plenty of info on Google/SO/etc.

Hi @brandones, I have improved it, see this here!
https://github.com/user-attachments/assets/570ed2c3-dbe4-4320-abbd-328e7491eddd

Thanks

brandones · 2024-10-22T20:15:19Z

That looks really nice @usamaidrsk , great work. Hopefully the UX reasoning is clear to you, for future work.

brandones

LGTM

brandones · 2024-10-22T20:19:40Z

I still think it might be nicer to have a higher limit for the number of locations to load. Feel free to merge this as it is, or to up the limit. Since loading takes so long it might be nice to minimize how many times the user has to wait for that loading to happen in order to find the location they want to select, which I figure would be accomplished by loading a longer list of locations each time.

brandones · 2024-10-22T20:19:53Z

(please post a video if you do change it)

usamaidrsk · 2024-10-22T20:39:28Z

(please post a video if you do change it)

I have increased the number of loactions loaded on searching from 5 to 10. Here is the video
https://github.com/user-attachments/assets/89da941a-a4dd-4f9d-a4e9-4d9054b269cf

brandones · 2024-10-22T20:43:14Z

Yeah I think that makes more sense to me. Thank you @usamaidrsk . And if someone wants to change that number again in the future it's easy enough.

usamaidrsk · 2024-10-22T20:48:23Z

Hopefully the UX reasoning is clear to you, for future work.

Thanks @brandones , it was great work here!
I will merge this now

usamaidrsk closed this Mar 13, 2024

usamaidrsk reopened this Mar 18, 2024

brandones changed the title ~~(feat) - O3-1831 Registration: Support person attribute of type Location~~ (feat) O3-1831: Registration: Support person attribute of type Location Jun 21, 2024

usamaidrsk requested review from brandones and vasharma05 July 18, 2024 20:42

mogoodrich requested review from mseaton and mogoodrich July 22, 2024 12:49

mseaton reviewed Jul 22, 2024

View reviewed changes

ibacher reviewed Jul 22, 2024

View reviewed changes

...rc/patient-registration/field/person-attributes/location-person-attribute-field.resource.tsx Outdated Show resolved Hide resolved

usamaidrsk force-pushed the ft-add-support-of-location-attribute branch from 294d56b to 014ab66 Compare September 23, 2024 06:17

usamaidrsk marked this pull request as ready for review September 27, 2024 16:14

usamaidrsk requested review from ibacher and mseaton September 27, 2024 16:14

brandones requested changes Oct 1, 2024

View reviewed changes

usamaidrsk requested a review from brandones October 2, 2024 18:18

usamaidrsk requested a review from denniskigen October 10, 2024 06:36

usamaidrsk force-pushed the ft-add-support-of-location-attribute branch from 7813ad8 to 92637ef Compare October 16, 2024 13:42

usamaidrsk added 12 commits October 22, 2024 19:35

ft-create-location-attribute component

c33f2af

feat: create person location attribute

2202275

(fix) remove useSWR in useCallback

05a2f57

(fix) update locationTag key description

522a472

(fix) selectedItem logic

6050b35

(fix) fix interface naming

66b4bda

add commonets on the onInputChange func

1f94c2d

(fix) add required param

e9cee5c

(fix) update location-tag description

cb1ec6b

(fix) use handleInputChange function naming

7ba2181

feat: add loading status for searching location

2f73d02

feat: append loading on the location attribute combobox

83d841b

usamaidrsk force-pushed the ft-add-support-of-location-attribute branch from 92637ef to 83d841b Compare October 22, 2024 16:58

brandones approved these changes Oct 22, 2024

View reviewed changes

usamaidrsk and others added 2 commits October 22, 2024 23:38

fix: increase number of locations loaded

de05e09

Merge branch 'main' into ft-add-support-of-location-attribute

f135d15

usamaidrsk merged commit fe14d60 into openmrs:main Oct 22, 2024
6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(feat) O3-1831: Registration: Support person attribute of type Location #1032

(feat) O3-1831: Registration: Support person attribute of type Location #1032

usamaidrsk commented Mar 13, 2024 •

edited

Loading

brandones commented Jun 21, 2024

brandones commented Jul 9, 2024

usamaidrsk commented Jul 10, 2024 •

edited by jira bot

Loading

brandones commented Jul 21, 2024

brandones commented Aug 19, 2024

brandones commented Sep 15, 2024

usamaidrsk commented Sep 18, 2024

usamaidrsk commented Sep 27, 2024

brandones Oct 1, 2024

brandones Oct 1, 2024

usamaidrsk Oct 2, 2024

brandones commented Oct 10, 2024

usamaidrsk commented Oct 16, 2024

brandones commented Oct 17, 2024

usamaidrsk commented Oct 17, 2024

ibacher commented Oct 17, 2024

usamaidrsk commented Oct 17, 2024

ibacher commented Oct 17, 2024

usamaidrsk commented Oct 17, 2024

brandones commented Oct 21, 2024

usamaidrsk commented Oct 22, 2024

brandones commented Oct 22, 2024

brandones left a comment

brandones commented Oct 22, 2024

brandones commented Oct 22, 2024

usamaidrsk commented Oct 22, 2024

brandones commented Oct 22, 2024

usamaidrsk commented Oct 22, 2024

(feat) O3-1831: Registration: Support person attribute of type Location #1032

(feat) O3-1831: Registration: Support person attribute of type Location #1032

Conversation

usamaidrsk commented Mar 13, 2024 • edited Loading

Requirements

Summary

Screenshots

Related Issue

Other

brandones commented Jun 21, 2024

brandones commented Jul 9, 2024

usamaidrsk commented Jul 10, 2024 • edited by jira bot Loading

brandones commented Jul 21, 2024

brandones commented Aug 19, 2024

brandones commented Sep 15, 2024

usamaidrsk commented Sep 18, 2024

usamaidrsk commented Sep 27, 2024

brandones Oct 1, 2024

Choose a reason for hiding this comment

brandones Oct 1, 2024

Choose a reason for hiding this comment

usamaidrsk Oct 2, 2024

Choose a reason for hiding this comment

brandones commented Oct 10, 2024

usamaidrsk commented Oct 16, 2024

brandones commented Oct 17, 2024

usamaidrsk commented Oct 17, 2024

ibacher commented Oct 17, 2024

usamaidrsk commented Oct 17, 2024

ibacher commented Oct 17, 2024

usamaidrsk commented Oct 17, 2024

brandones commented Oct 21, 2024

usamaidrsk commented Oct 22, 2024

brandones commented Oct 22, 2024

brandones left a comment

Choose a reason for hiding this comment

brandones commented Oct 22, 2024

brandones commented Oct 22, 2024

usamaidrsk commented Oct 22, 2024

brandones commented Oct 22, 2024

usamaidrsk commented Oct 22, 2024

usamaidrsk commented Mar 13, 2024 •

edited

Loading

usamaidrsk commented Jul 10, 2024 •

edited by jira bot

Loading