-
Notifications
You must be signed in to change notification settings - Fork 62
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
(feat) Introduce Location Datasource #113
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work @kajambiya. Just a few minor comments
src/components/inputs/ui-select-extended/ui-select-extended.tsx
Outdated
Show resolved
Hide resolved
src/components/inputs/ui-select-extended/ui-select-extended.tsx
Outdated
Show resolved
Hide resolved
src/components/inputs/ui-select-extended/ui-select-extended.tsx
Outdated
Show resolved
Hide resolved
6789951
to
7bc1b5a
Compare
import { DataSource } from '../api/types'; | ||
|
||
export class LocationDataSource implements DataSource<OpenmrsResource> { | ||
private initialUrl: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you need this?
src/datasources/data-source.ts
Outdated
|
||
toUuidAndDisplay(data: OpenmrsResource): OpenmrsResource { | ||
if (typeof data.uuid === 'undefined' || typeof data.display === 'undefined') { | ||
throw new Error(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you provide an error message? Something like:
throw new Error("'uuid' or 'display' not found in the OpenMRS object.");
} | ||
|
||
fetchData(searchTerm: string, config?: Record<string, any>): Promise<any[]> { | ||
this.url = this.initialUrl; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there cases where you envision the URL changing for this Datasource? Why parameterize it? Why not fuse it within assuming this Datasource will only be used for locations? Ideally, it should accept adhoc query params through configurations and the search term but I'm not sure I understand why we require a URL in the constructor.
The parameter only makes sense for data sources whose URL might change like the base data source.
7bc1b5a
to
e4aacb0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks @kajambiya
Requirements
Summary
This PR introduces the location data source. It allows locations to be captured as obs with the help of the ui-select-extended control.
The PR also includes some code refactor to the data-source component.
Screenshots
Screen.Recording.2023-08-22.at.09.17.25.mov
Related Issue
Other