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

OBPIH-6520 Set up for date localization in DD/Month/YYYY format for Es and Mx locales #4682

Closed
wants to merge 9 commits into from

Conversation

alannadolny
Copy link
Collaborator

Usage:

  1. G tag
<g:formatDate date="${new Date()}" formatName="${org.pih.warehouse.DateFormatName.CUSTOM.id}"/>
<g:formatDate date="${new Date()}" formatName="${org.pih.warehouse.DateFormatName.DEFAULT.id}"/>
<g:formatDate date="${new Date()}" formatName="${org.pih.warehouse.DateFormatName.EXPIRY.id}"/>

  1. Grails util
LocalizationUtil.formatDate(new Date(), DateFormatName.EXPIRY)
LocalizationUtil.formatDate(new Date(), DateFormatName.CUSTOM)
LocalizationUtil.formatDate(new Date(), DateFormatName.DEFAULT)
  1. React component
<FormatDate date={new Date()} formatName={DateFormatName.DEFAULT} />
<FormatDate date={new Date()} formatName={DateFormatName.EXPIRY} />
<FormatDate date={new Date()} formatName={DateFormatName.CUSTOM} />

# 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
Copy link
Collaborator

@EWaterman EWaterman Jun 21, 2024

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?

Copy link
Collaborator

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'),
Copy link
Collaborator

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

Copy link
Collaborator

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
Copy link
Collaborator

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'),
Copy link
Collaborator

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.

@jmiranda jmiranda added the warn: do not merge Marks a pull request that is not yet ready to be merged label Jun 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
warn: do not merge Marks a pull request that is not yet ready to be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants