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

(feat) O3-2491: Add support for a time picker #312

Merged
merged 9 commits into from
Jun 12, 2024
Merged

Conversation

NethmiRodrigo
Copy link
Contributor

@NethmiRodrigo NethmiRodrigo commented Jun 7, 2024

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 aims to support the rendering of a time picker alongside the date picker based on the prop datePickerFormat. If the prop is null, only the date picker will be displayed.

Screenshots

Screenshot 2024-06-07 at 5 24 30 PM

Submitting values

Screen.Recording.2024-06-12.at.11.56.03.AM.mov

Editing time

Screen.Recording.2024-06-12.at.11.57.03.AM.mov

Related Issue

JIRA ticket

Other

PR to update prop name

Copy link

github-actions bot commented Jun 7, 2024

Size Change: +372 B (+0.04%)

Total Size: 1.05 MB

ℹ️ View Unchanged
Filename Size Change
dist/184.js 11.2 kB 0 B
dist/225.js 2.57 kB 0 B
dist/29.js 163 kB 0 B
dist/300.js 709 B 0 B
dist/31.js 5.32 kB 0 B
dist/318.js 2.43 kB 0 B
dist/327.js 1.84 kB 0 B
dist/335.js 967 B 0 B
dist/353.js 3.02 kB 0 B
dist/371.js 71.5 kB 0 B
dist/41.js 3.36 kB 0 B
dist/436.js 759 B 0 B
dist/465.js 182 kB 0 B
dist/540.js 2.63 kB 0 B
dist/55.js 2.14 kB 0 B
dist/635.js 14.3 kB 0 B
dist/8.js 3.68 kB 0 B
dist/800.js 245 kB +204 B (+0.08%)
dist/979.js 6.85 kB 0 B
dist/99.js 690 B 0 B
dist/993.js 3.09 kB 0 B
dist/998.js 15.6 kB 0 B
dist/main.js 300 kB +168 B (+0.06%)
dist/openmrs-form-engine-lib.js 3.76 kB 0 B

compressed-size-action

Copy link
Member

@samuelmale samuelmale left a comment

Choose a reason for hiding this comment

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

For cases where datePickerFormat is set to "timer", what kind of value will be submitted to the backend? @NethmiRodrigo Can you share a sample payload of an obs?

src/types.ts Outdated
@@ -261,6 +262,7 @@ export type RenderType =
| 'content-switcher'
| 'date'
| 'datetime'
| 'time'
Copy link
Member

Choose a reason for hiding this comment

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

Here you're introducing a new rendering that is supported through the "date" rendering. With the introduction of the datePickerFormat prop, we only need one rendering "date". Meaning we should consider removing or deprecating the "datetime" rendering.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adding time was a mistake so I removed it. Leaving datetime as it is for now as it has being referenced in a couple of places.

Copy link
Member

Choose a reason for hiding this comment

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

I think we need to update existing references to ensure that we have a single source of truth. Eg. We should use datePickerFormat here to determine the date-picker type.

@NethmiRodrigo
Copy link
Contributor Author

NethmiRodrigo commented Jun 7, 2024

For cases where datePickerFormat is set to "timer", what kind of value will be submitted to the backend? @NethmiRodrigo Can you share a sample payload of an obs?

{ "value": "2024-05-27", "concept": "81a50873-81c8-4c2e-8892-b2cd635b704c", "formFieldNamespace": "rfe-forms", "formFieldPath": "rfe-forms-__tyxtEosu2" }

Seems like only the date is going through at the moment (when the input was both date and time), trying to fix it now.

@brandones brandones changed the title (feat):O3-2491: Add support for time rendering (feat): O3-2491: Add support for a time picker Jun 11, 2024
@brandones brandones changed the title (feat): O3-2491: Add support for a time picker (feat) O3-2491: Add support for a time picker Jun 11, 2024
@samuelmale
Copy link
Member

@NethmiRodrigo can you attach a video or GIF demonstrating a scenario where the date-picker type is set to "timer" in enter vs edit mode?

@NethmiRodrigo
Copy link
Contributor Author

@NethmiRodrigo can you attach a video or GIF demonstrating a scenario where the date-picker type is set to "timer" in enter vs edit mode?

@samuelmale updated the description with a screen recording of submitting and editing

Copy link
Member

@samuelmale samuelmale 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 @NethmiRodrigo!

@samuelmale samuelmale merged commit 6567d76 into main Jun 12, 2024
4 checks passed
@samuelmale samuelmale deleted the feat/O3-2491 branch June 12, 2024 22:11
@denniskigen
Copy link
Member

denniskigen commented Jun 21, 2024

Is the consensus that we're keeping the datetime rendering type around for compatibility reasons? I'm asking w.r.t openmrs/openmrs-esm-form-builder#289.

@samuelmale
Copy link
Member

It's fair to treat datetime as a deprecated rendering type. But there is backwards compatibility see.

@denniskigen
Copy link
Member

Also, it looks like labels for DatePicker components aren't getting rendered (snippet from the A New Sample Form for Testing schema on dev3):

CleanShot 2024-06-24 at 4  12 58@2x

@NethmiRodrigo
Copy link
Contributor Author

Also, it looks like labels for DatePicker components aren't getting rendered (snippet from the A New Sample Form for Testing schema on dev3):

CleanShot 2024-06-24 at 4  12 58@2x

Hmm very weird. I'll take a look

@samuelmale
Copy link
Member

This is related to this openmrs/openmrs-esm-core#1041. The labelText prop was renamed to label

@ibacher
Copy link
Member

ibacher commented Jun 24, 2024

I've restored the labelText attribute and deprecated the label one.

@has9h
Copy link

has9h commented Jun 26, 2024

Hi @denniskigen @NethmiRodrigo @samuelmale sorry for tagging you all, but I'm not sure what the corresponding PR for this is in the form-builder repo; we will need this into the form-builder to render the time picker, correct?

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.

5 participants