-
-
Notifications
You must be signed in to change notification settings - Fork 403
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
OBPIH-6520 Set up for date localization in DD/Month/YYYY format for Es and Mx locales #4682
Conversation
grails-app/i18n/messages.properties
Outdated
# Date formats | ||
react.default.defaultDate.format=MM/DD/yyyy hh:mm:ss a z | ||
react.default.customDate.format=MM/DD/yyyy | ||
react.default.expiryDate.format=MM/yyyy |
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.
we're only changing the format to dd/MMM/yyyy for ex_MX? Do we not want all locals to have the new format?
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.
I think that others should have a default right now.
enum DateFormatName { | ||
DEFAULT('default.date.format'), | ||
CUSTOM('custom.date.format'), | ||
EXPIRY('expiry.date.format'), |
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.
I'm not sure if this is the types we want. DEFAULT and CUSTOM are more generic sounding while EXPIRY is for a very specific purpose (product expiration). It's mixing and matching a bit.
What are you imagining is the behaviour if we add a new feature X that also needs to display only month and year? Would it re-use the EXPIRY type or would it need to create its own X type that also formats MMM/yyyy?
My instinct tells me that we'd be better off with more directly descriptive types (ex: DATE_TIME, FULL_DATE. MONTH_YEAR) but let me know your thoughts
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.
and if you want to avoid having DEFAULT be in the enum itself you could move it to a constant in the enum:
public final DateFormatName DEFAULT = DATE_TIME
CUSTOM('custom.date.format'), | ||
EXPIRY('expiry.date.format'), | ||
|
||
final String id |
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.
I think something like "property" is more accurate of a field name
enum DateFormatName { | ||
DATE_TIME('default.date.format'), | ||
FULL_DATE('custom.date.format'), | ||
MONTH_YEAR('expiry.date.format'), |
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.
thanks for making these changes but now the actual properties themselves don't match.
I was thinking we'd have something like:
datetime.format=DD/MMM/yyyy hh:mm:ss a z
date.format=DD/MMM/yyyy
monthyear.format=MMM/yyyy
the monthyear one is hard to decide on what a good wording would be but maybe it's clear enough like that.
Usage: