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-3632 - Add config for maxDate and minDate to DateObsField #1261

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

kajambiya
Copy link
Contributor

@kajambiya kajambiya commented Jul 31, 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 allows users to pass max and min dates to the DateObsField via config

Screenshots

Screen.Recording.2024-10-29.at.19.25.09.mov

Related Issue

Other

Copy link
Member

@ibacher ibacher left a comment

Choose a reason for hiding this comment

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

Thanks @kajambiya! A few things need to be cleaned up.

return new Date();
}

const date = new Date(dateString);
Copy link
Member

Choose a reason for hiding this comment

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

This would be a great place to use dayjs...

Comment on lines 176 to 181
if (!isNaN(date.getTime())) {
return date;
}
Copy link
Member

Choose a reason for hiding this comment

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

Could you explain this a bit better?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was to check whether the string was successfully converted into a date but since you advised a try...catch, this is no longer needed.

return date;
}

return 'Invalid min or max date!';
Copy link
Member

Choose a reason for hiding this comment

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

  1. This is a user-facing string so it must be localizable
  2. Make the message friendlier by passing in as an argument whether this is the min date or max date
  3. Instead of checking the return type, throw an error and use try...catch. It may be better to actually create the error message at that point.

@@ -12,7 +12,8 @@ export interface FieldDefinition {
label?: string;
uuid: string;
placeholder?: string;
Copy link
Member

Choose a reason for hiding this comment

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

May as well remove placeholder here too? Also, these should be removed from the actual config schema and not just the types.

@brandones
Copy link
Contributor

Ping @kajambiya . Is it clear to you how to move forward with this?

@kajambiya
Copy link
Contributor Author

Ping @kajambiya . Is it clear to you how to move forward with this?

Yes it is @brandones . Was just busy with some other tasks and had packed this but going to wire a commit tomorrow.

@kajambiya
Copy link
Contributor Author

kajambiya commented Sep 27, 2024

Thanks @kajambiya! A few things need to be cleaned up.

Hi @ibacher
I've addressed the comments.
Just a quick one, I'm logging the thrown error but I'm also going ahead to pass it as a field error(See video). Is this necessary or the log should be enough?

@ibacher
Copy link
Member

ibacher commented Sep 30, 2024

@kajambiya As a general rule, the most important thing is that if there is an error and it blocks the user from taking an action, that error must be shown to the user in a way they can understand and (hopefully) correct. So the general answer to your question is that logging to the console is a nice-to-have, but not a sufficient implementation of handling an error.

@brandones
Copy link
Contributor

@ibacher Does that mean this is in good shape? It looks like the configDateError has been added to the display using the invalidText property of the datepicker.

@ibacher ibacher changed the title (chore) O3-3632 Add config for maxDate and minDate to DateObsField (feat) O3-3632 Add config for maxDate and minDate to DateObsField Oct 1, 2024
@ibacher ibacher changed the title (feat) O3-3632 Add config for maxDate and minDate to DateObsField (feat) O3-3632 - Add config for maxDate and minDate to DateObsField Oct 1, 2024
const parsedDate = dayjs(dateString);
if (!parsedDate.isValid()) {
throw new Error(
translateFrom(moduleName, 'invalidObsDateErrorMessage', `The ${dateType} date value provided is invalid!`),
Copy link
Member

Choose a reason for hiding this comment

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

  1. Why are we using translateFrom here instead of t?
  2. We can't interpolate Min and Max in here like this. It's not translatable. We can do something like t('invalidObsDateMinValue', 'The minimum date value provided is invalid.')

Copy link
Member

Choose a reason for hiding this comment

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

It would, of course, be better to include some example of what is valid.

}

function DateObsField({ concept, label, required, placeholder }: DateObsFieldProps) {
const evaluateConfigDate = (dateString: string, dateType: string): Date | null => {
Copy link
Member

Choose a reason for hiding this comment

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

So basically, the real feature being supported by this PR is the ability to specify today as either the lowest selectable date or the highest selectable date?

Copy link
Member

Choose a reason for hiding this comment

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

Basically, the question here is "why don't we just build that instead of this?"

Copy link
Contributor

Choose a reason for hiding this comment

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

+1, I would support a config interface like, for example,

datepicker: {
  allowPastDates: boolean
  allowFutureDates: boolean
}

does what's actually needed, is easier to maintain and code, and easier to understand and use.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ibacher & @brandones, my question here would be, don't we anticipate a scenario where someone would pass a fixed date e.g 2024-10-01 as either the min or max date. I don't have a use case per se but I just thought it would be possible.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I just can't really imagine a use case for that. It is good not to build things no one needs, or to design and build things with no use case. It is valuable to keep our code easy to understand, maintain, and use. If someday a use case comes up, we'll implement it then.

Copy link
Member

Choose a reason for hiding this comment

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

@ibacher / @brandones / @kajambiya please keep in mind that "today" from the perspective of a field on a form often needs to be "as of the encounter date" not "as of the entry date". So it is not sufficient to say we are only going to support past and future as of a "new Date()" object. So this might be an example of where a parent component (eg. a parent form with an encounterDate variable would want to set that as the minDate / maxDate / defaultDate on an obsField it contains). Sorry if this is not relevant, but wanted to mention it in case it is.

Copy link
Member

Choose a reason for hiding this comment

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

@mseaton Yeah, retrospective registration probably requires a redesign of the app. Here, though, we're just passing in a value from the configuration, so the issue is kind of orthogonal to that concern (i.e., we could implement this so that it interprets "today" relative to the user's current selected time for the registration to happen). The issue here is that there's isn't a lot of use for handling strings like '2020-01-05'.

return null;
}
if (dateString === 'today') {
return dayjs(new Date()).toDate();
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return dayjs(new Date()).toDate();
return new Date();

Comment on lines 14 to 15
placeholder?: string;
dateFormat?: string;
maxDate?: string;
minDate?: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

You modified the interface but did not change the actual config schema. Also why did you get rid of placeholder?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had removed it because the placeholder for the datepicker is now defined by the openMRSDatepicker component depending on the locale but I've realized that placeholder usage is not for datepicker alone so I'm going to add it back.

@brandones
Copy link
Contributor

@kajambiya is it clear how to move forward?

@kajambiya
Copy link
Contributor Author

@kajambiya is it clear how to move forward?

@brandones Here's how I think we should proceed. Let me know if you have any thoughts:

Remove the minDate and maxDate.
Introduce two flags: allowFutureDates and allowPastDates.
By default, both flags will be set to true, allowing users to select any date. Implementers can then configure whether to allow or restrict future or past dates as needed.

@brandones
Copy link
Contributor

Sounds good, thanks @kajambiya

@kajambiya
Copy link
Contributor Author

Sounds good, thanks @kajambiya

@brandones @ibacher, I've updated the code and the video accordingly. Kindly review and revert.

@brandones
Copy link
Contributor

Sorry, I thought we were going to do allowFutureDates and allowPastDates? The reason I think this is more intuitive is because the dates are not being "enabled" or "disabled," their selection is being "allowed" or "disallowed." But "disallow" is uncommon in interfaces, so setting "allowFutureDates" to false is, I think, the intuitive interface for this.

@kajambiya
Copy link
Contributor Author

Sorry, I thought we were going to do allowFutureDates and allowPastDates? The reason I think this is more intuitive is because the dates are not being "enabled" or "disabled," their selection is being "allowed" or "disallowed." But "disallow" is uncommon in interfaces, so setting "allowFutureDates" to false is, I think, the intuitive interface for this.

Hi @brandones Sorry for the late response. I've updated the code and the video accordingly

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.

4 participants