O3-4740: Refactor Location Selector Component for Reusability and Move to esm-core Styleguide #1368
Conversation
Bawanthathilan
changed the title
jayasanka-sack
left a comment
jayasanka-sack
left a comment
There was a problem hiding this comment.
Thanks for getting this started, @Bawanthathilan! I know it’s still a WIP, but I’ve left a few comments for you. Appreciate your work on this so far!
| defaultLocationUuid?: string; | ||
| locationTag?: string; | ||
| locationsPerRequest?: number; | ||
| onChange: (locationUuid?: string) => void; |
There was a problem hiding this comment.
| onChange: (locationUuid?: string) => void; | |
| onChange: (locationUuid: string) => void; |
locationUuid shouldn't be null
packages/framework/esm-styleguide/src/location-selector/location-selector.component.tsx
Outdated
Show resolved
Hide resolved
packages/framework/esm-styleguide/src/location-selector/location-selector.component.tsx
Outdated
Show resolved
Hide resolved
packages/framework/esm-styleguide/src/location-selector/location-selector.component.tsx
Outdated
Show resolved
Hide resolved
|
Hi @jayasanka-sack @ibacher can you review this PR |
|
Thanks a lot for getting this done @Bawanthathilan! would you mind adding some screenshots and/or a video recording when you get a chance? |
packages/framework/esm-styleguide/src/location-selector/location-selector.component.tsx
Outdated
Show resolved
Hide resolved
packages/framework/esm-styleguide/src/location-selector/location-selector.component.tsx
Outdated
Show resolved
Hide resolved
| }, [fetchedLocations]); | ||
|
|
||
| const effectiveLocations = useMemo<Location[]>(() => { | ||
| const base = fetchedLocations.length > 0 ? fetchedLocations : tempLocations; |
There was a problem hiding this comment.
When the user searches for a non-existing location, fetchedLocations will be empty, and the component will fall back to tempLocations. However, in this case the expected behavior is to show an empty list. Can you confirm if that’s what currently happens?
There was a problem hiding this comment.
Also, could we rename the variable base to something more descriptive of what it actually holds? That would make the code easier to follow.
There was a problem hiding this comment.
Were you able to find a way to resolve this one?
packages/framework/esm-styleguide/src/location-selector/location-selector.component.tsx
Show resolved
Hide resolved
| initialSelectedItem={items.find((i) => i.id === defaultLocationUuid)} | ||
| selectedItem={items.find((i) => i.id === selectedLocationUuid)} |
There was a problem hiding this comment.
when selectedItem is provided, the ComboBox behaves as a controlled component, so initialSelectedItem is ignored after the first render. To keep things clearer and avoid confusion it might be better to remove initialSelectedItem here and rely only on selectedItem. What do you think?
| import React, { useState, useEffect, useMemo } from 'react'; | ||
| import { useTranslation } from 'react-i18next'; | ||
| import { ComboBox } from '@carbon/react'; | ||
| import { useLocationByUuid, useLocations } from '../location-picker/location-picker.resource'; |
There was a problem hiding this comment.
Data fetching hooks should be moved to esm-react-utils and not left in the styleguide.
| import { useTranslation } from 'react-i18next'; | ||
| import { ComboBox } from '@carbon/react'; | ||
| import { useLocationByUuid, useLocations } from '../location-picker/location-picker.resource'; | ||
| import { useDebounce } from '@openmrs/esm-react-utils'; |
There was a problem hiding this comment.
This should be above local imports.
| <ComboBox | ||
| aria-label={label} | ||
| id="location" | ||
| invalidText={t('Required')} |
There was a problem hiding this comment.
We can't translated text like this in the styleguide. Please use the getCoreTranslation() function from esm-translations.
|
From the test failures, it doesn't look like this PR correctly re-creates the existing location selector. That needs to be fixed. |
Thanks for reviewing @ibacher @jayasanka-sack ill fixed those changes |
4 participants
Requirements
feat,fix, orchore, among others). See existing PR titles for inspiration.For changes to apps
If applicable
Summary
The previous implementation of the location component was not reusable and had UX limitations. To address these issues, the component was refactored into a reusable LocationSelector and moved to the esm-core styleguide for broader use across projects.
Screenshots
Screen.Recording.2025-10-06.at.8.32.23.AM.mov
Related Issue
https://openmrs.atlassian.net/browse/O3-4740
Other