Skip to content
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

Added kenyaemr providers esm #289

Closed
wants to merge 8 commits into from
Closed

Conversation

Rugute
Copy link
Contributor

@Rugute Rugute commented Jul 29, 2024

Requirements

  • This PR has a title that briefly describes the work done, including the ticket number if there is a ticket.
  • My work conforms to the OpenMRS 3.0 Styleguide.
  • I checked for feature overlap with existing widgets.

Summary

Added Providers ESM

Screenshots

Screenshot 2024-07-24 at 12 13 59

None.

Related Issue

None.

Other

None.

Copy link
Contributor

@donaldkibet donaldkibet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @Rugute for this great work. Left some suggestions

packages/esm-providers-app/src/index.ts Outdated Show resolved Hide resolved
Comment on lines +22 to +25
const [searchResults, setSearchResults] = useState<any[]>([]);
const [selectedUser, setSelectedUser] = useState<any | null>(null);
const [licenseNumber, setLicenseNumber] = useState('');
const [expiryDate, setExpiryDate] = useState('');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already using react-hook-form it would be nice to use it here

if (response) {
setSearchResults(response);
} else {
console.error('Error fetching users:');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should be notifying the user about any errors

Comment on lines +9 to +20
const fetcher = async (url) => {
try {
const response = await openmrsFetch(url, {});
if (!response.ok) {
throw new Error(`Failed to fetch data: ${response.statusText}`);
}

return response.json();
} catch (error) {
throw new Error(`An error occurred while fetching data: ${error.message}`);
}
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need to define this fetcher if we are hitting openmrs endpoints openmrsfetch will just work fine

Comment on lines +1 to +3
@use '@carbon/styles/scss/spacing';
@use '@carbon/styles/scss/type';
@import '~@openmrs/esm-styleguide/src/vars';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use as pointed above


function ProviderMetrics() {
const { t } = useTranslation();
const { response } = getAllProviders();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should use useSWRImmutable to cache this aggressively since this data doesn't change very often

min-width: 8.875rem;

&:active {
outline: 2px solid var(--brand-03) !important;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are using !important here

@Rugute
Copy link
Contributor Author

Rugute commented Jul 29, 2024

I am working on the changes. Pushing it back shotly

@donaldkibet
Copy link
Contributor

Thanks @Rugute am closing this since i was unable to push some changes to the branch. I have migrated the changes to this PR. Feel appreciated for your work on this 🤝
Opened PR #305

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants