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

(refactor) O3-4347: Migrate queue forms to react-hook-form and zod #1442

Conversation

Muppasanipraneeth
Copy link
Contributor

@Muppasanipraneeth Muppasanipraneeth commented Jan 15, 2025

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 refactors several forms in the Service Queues app to use React Hook Form and Zod. These forms include the following:

  • Add provider to queue room form
  • Add new queue service room form

This PR:

  • Migrates form handling concerns to React Hook Form and zod schema validation replacing manual form state.
  • Converts uncontrolled form components to controlled components
  • Improves error handling and user feedback
  • Adds proper TypeScript types for form data
  • Adapts test coverage to cover the new behavior

Screenshots

Before

Screenshot 2025-01-14 at 6 56 33 PM Screenshot 2025-01-21 at 6 47 43 PM

After

Screenshot 2025-01-21 at 6 46 24 PM Screenshot 2025-01-15 at 9 48 04 AM

Related Issue

https://issues.openmrs.org/browse/O3-4347

Other

@Muppasanipraneeth Muppasanipraneeth marked this pull request as draft January 15, 2025 12:01
@Muppasanipraneeth
Copy link
Contributor Author

@denniskigen

@Muppasanipraneeth Muppasanipraneeth changed the title (refactor) : 03-4347 Refactor queue service form with react-hook-form and zod (refactor) o3-4347 :Refactor queue service form with react-hook-form and zod Jan 21, 2025
@denniskigen
Copy link
Member

Please add a description to your PR and format the PR title to match the convention.

@Muppasanipraneeth Muppasanipraneeth changed the title (refactor) o3-4347 :Refactor queue service form with react-hook-form and zod (refactor) O3-4347: Refactor queue service form with react-hook-form and zod Jan 21, 2025
@Muppasanipraneeth
Copy link
Contributor Author

Please add a description to your PR and format the PR title to match the convention.

updated @denniskigen

@Muppasanipraneeth Muppasanipraneeth marked this pull request as ready for review January 21, 2025 13:33
@denniskigen
Copy link
Member

Unrelated pro-tip: shorter branch names are always better.

Copy link
Member

@denniskigen denniskigen left a comment

Choose a reason for hiding this comment

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

Thanks for kicking this off, @Muppasanipraneeth. I've left some comments.

const onSubmit = async (data: QueueRoomFormData) => {
try {
// here doubt why queueName twice // name: string, description: string, queueUuid: string
const response = await saveQueueRoom(data.queueRoomName, data.queueRoomName, data.queueRoomService);
Copy link
Member

@denniskigen denniskigen Jan 28, 2025

Choose a reason for hiding this comment

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

Per this liquibase file, description is a String value that represents "an optional description for the queue room".

While I understand why the original author defaulted to using the queue room name as the description (since we're not explicitly collecting this information from the user), I don't see any reason why we shouldn't collect a proper description. @chibongho, @donaldkibet are you aware of any specific reason why we don't have a queue room description field in the form?

@denniskigen denniskigen changed the title (refactor) O3-4347: Refactor queue service form with react-hook-form and zod (refactor) O3-4347: Migrate queue forms to react-hook-form and zod Jan 29, 2025
@Muppasanipraneeth
Copy link
Contributor Author

Screenshot 2025-01-29 at 8 09 49 PM
  • as of now not passing any thing to subtitle=""
{errorFetchingQueueRooms && (
    <InlineNotification  
      className={styles.errorNotification}
      kind="error"
      onClick={() => {}}
      subtitle={""}
      title={t('errorFetchingQueueRooms', 'Error fetching queue rooms')}
    />
  )} 

@Muppasanipraneeth
Copy link
Contributor Author

this is after adding inlineloading

Screenshot 2025-01-29 at 9 11 52 PM

@denniskigen denniskigen force-pushed the O3-4347-use-react-hook-form-and-zod-in-the-queue-service-form-for-new-service-room-and-provider-queue-room branch from 32f1e4b to 90178d7 Compare January 30, 2025 17:56
Copy link
Member

@denniskigen denniskigen left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks, @Muppasanipraneeth!

@denniskigen denniskigen merged commit 9555252 into openmrs:main Jan 30, 2025
6 checks passed
Samstar10 pushed a commit to Samstar10/openmrs-esm-patient-management that referenced this pull request Feb 4, 2025
…penmrs#1442)

* added zod and react from hook

* update room selection test assertions

* Refactor form validation logic with Zod and React hooks

* Update packages/esm-service-queues-app/src/add-provider-queue-room/add-provider-queue-room.component.tsx

* fixed test

* updated packages/esm-service-queues-app/src/queue-rooms/queue-room-form.workspace.tsx

* updated en.json

* added inlinenotification for fetching queue room

* added notification style

* update return of the useQueueRooms hook

* updated test

* updated test

* added inline loding for submit button and standralize field name

* updated en.json

* refactor: standardize error messages and improve accessibility in queue room forms

* Minor tweaks

---------

Co-authored-by: Dennis Kigen <[email protected]>
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