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

Standardized Date and Time #5533

Closed

Conversation

Saumyachitransh01
Copy link
Member

Fixes #5490

Short description of what this resolves:

All the occurrence of dates and time has been converted to only 5 formats

Changes proposed in this pull request:

Since there were more than 20 formats used all over the platform, I've minimized it to use only 5 formats.
Five formats that are used everywhere including all .js files and .hbs files are:-

  1. DD-MM-YYYY
  2. ddd, DD MMMM, YYYY
  3. hh:mm A
  4. ddd, DD MMMM, YYYY hh:mm A
  5. DD MMMM, YYYY hh:mm A

Checklist

  • I have read the Contribution & Best practices Guide.
  • My branch is up-to-date with the Upstream development branch.
  • The acceptance, integration, unit tests and linter pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

@vercel
Copy link

vercel bot commented Nov 9, 2020

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/eventyay/open-event-frontend/lc0wwjz9w
✅ Preview: https://open-event-frontend-git-branchnew.eventyay.vercel.app

@@ -116,10 +116,10 @@ export default Component.extend(FormMixin, EventWizardMixin, {
this.oldTimezone = this.data.event.timezone;
if (!oldTimezone || !this.oldTimezone || oldTimezone === this.oldTimezone) {return}
if (event.startsAt) {
event.startsAt = moment.tz(event.startsAt.clone().tz(oldTimezone).format('YYYY-MM-DDTHH:mm:ss.SSS'), moment.ISO_8601, this.data.event.timezone);
event.startsAt = moment.tz(event.startsAt.clone().tz(oldTimezone).format('DD MMMM, YYYY hh:mm A'), moment.ISO_8601, this.data.event.timezone);
Copy link
Member

Choose a reason for hiding this comment

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

This is not user facing format. This is ISO8601 format and it needs to remain same

@@ -15,10 +15,10 @@
@viewName="agendaDay"
@defaultView="timelineDay"
@validRange={{this.validRange}}
@now={{moment-format (now) "YYYY-MM-DD"}}
@now={{moment-format (now) "DD-MM-YYYY"}}
Copy link
Member

Choose a reason for hiding this comment

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

This is not user facing format. This needs to be passed in the exact same way to the component

@scrollTime="0:00:00"
@slotDuration="00:05:00"
@slotLabelFormat="HH:mm"
@slotLabelFormat="hh:mm A"
Copy link
Member

Choose a reason for hiding this comment

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

Same. This should not be changed

@Saumyachitransh01
Copy link
Member Author

Okay, and apart from these three are there any other corrections needed?

@iamareebjamal
Copy link
Member

@mariobehling Please check

Copy link
Member

@mariobehling mariobehling left a comment

Choose a reason for hiding this comment

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

Thank you very much! This is a huge improvement and seeing your changes I believe it would be even better, if we could further limit this to only two formats. Here is the list below.

Please do not add a "0" in front of single digit date or time formats.

  1. This is good for tables:
    Make event format as in this example: 4 Dec, 2020 5:00 PM (IST)
  1. This is good for most public pages
    Make event format as in this example: Friday, 4 December, 2020 5:00 PM (IST)
  1. Am I missing something important? Please share if any.

@Saumyachitransh01
Copy link
Member Author

Screenshot from 2020-11-10 05-52-07

And places like these? Having the format "MM/DD/YYYY". Which one of those two formats should I use?

@iamareebjamal
Copy link
Member

4 Dec, 2020

@@ -18,7 +18,7 @@
@now={{moment-format (now) "YYYY-MM-DD"}}
@scrollTime="0:00:00"
@slotDuration="00:05:00"
@slotLabelFormat="HH:mm"
@slotLabelFormat="HH:MM"
Copy link
Member

Choose a reason for hiding this comment

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

Again, this is not for public display.

@@ -465,7 +465,7 @@ export default Component.extend(FormMixin, EventWizardMixin, {
eventStartDate = moment(new Date(eventStartDate));
this.data.event.tickets.forEach(ticket => {
if (moment(eventStartDate).isBefore(ticket.get('salesEndsAt'))) {
ticket.set('salesEndsAt', moment(eventStartDate, 'MM/DD/YYYY').toDate());
ticket.set('salesEndsAt', moment(eventStartDate, 'DD-MMMM-YYYY').toDate());
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, where did this format come from? And this is not for public display

@iamareebjamal
Copy link
Member

Please don't change formats which are used for parsing and not for display

@Saumyachitransh01
Copy link
Member Author

Please don't change formats which are used for parsing and not for display

So should I only change hbs and not js?

@iamareebjamal
Copy link
Member

JS contains display formats as well. And the build is failing. The initial work was much more stable. I don't know what triggered the need for editing each file one at a time. You already had another PR ready. You could just force push the changes on this branch

@iamareebjamal
Copy link
Member

I have forced pushed the changes from other PR to this. Please delete the local branch and fetch again to continue the work.

git checkout development
git branch -D branchNew
git fetch origin branchNew
git checkout branchNew

@Saumyachitransh01
Copy link
Member Author

Please don't change formats which are used for parsing and not for display

It's a bit confusing, how to determine which one is for parsing and which is for display
Screenshot from 2020-11-11 17-19-58
Please confirm if this is for display or not?

@iamareebjamal
Copy link
Member

It is a display format

@mariobehling
Copy link
Member

Hi, would be great if this PR gets finalized. As this PR is blocking others from resolving the issue, we would close it in the upcoming days if there is no activity. You already contributed a good amount of time to this PR, so I think with some more effort you could finalize it. Keep it up!

@codecov
Copy link

codecov bot commented Nov 22, 2020

Codecov Report

Merging #5533 (a584d34) into development (7c4f3e8) will decrease coverage by 0.11%.
The diff coverage is n/a.

Impacted file tree graph

@@               Coverage Diff               @@
##           development    #5533      +/-   ##
===============================================
- Coverage        23.71%   23.60%   -0.12%     
===============================================
  Files              510      510              
  Lines             5422     5422              
  Branches            59       59              
===============================================
- Hits              1286     1280       -6     
- Misses            4121     4126       +5     
- Partials            15       16       +1     
Impacted Files Coverage Δ
app/controllers/account/billing/invoices/list.js 100.00% <ø> (ø)
app/controllers/admin/messages/list.js 0.00% <ø> (ø)
app/controllers/admin/sales/invoices.js 0.00% <ø> (ø)
app/controllers/admin/sessions/list.js 0.00% <ø> (ø)
app/controllers/admin/users/list.js 0.00% <ø> (ø)
app/controllers/events/list.js 23.07% <ø> (ø)
app/controllers/events/view/sessions/list.js 0.00% <ø> (ø)
app/controllers/events/view/tickets/orders/list.js 0.00% <ø> (ø)
app/components/tabbed-navigation.js 33.33% <0.00%> (-20.00%) ⬇️
app/services/cache.ts 64.15% <0.00%> (-5.67%) ⬇️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7c4f3e8...a584d34. Read the comment docs.

@@ -3,7 +3,7 @@ import moment from 'moment';

export function headerDate(params) {
const timezone = params[1] ? params[1] : moment.tz.guess();
return `${moment(params[0]).tz(timezone).format('dddd, D MMMM, YYYY h:mm A')} (${moment.tz(params[0], timezone).zoneAbbr()})`;
return `${moment(params[0]).tz(timezone).format('dddd, MMMM Do YYYY, h:mm A')} (${moment.tz(params[0], timezone).zoneAbbr()})`;
Copy link
Member

Choose a reason for hiding this comment

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

Why reverted the format here? We need same format everywhere

Copy link
Member Author

Choose a reason for hiding this comment

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

This was the only way for the build to be successful.
Changing it was failing the build every time.

Copy link
Member

Choose a reason for hiding this comment

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

No, that's not the only way to fix the build

@mariobehling
Copy link
Member

This has been resolved already elsewhere now. Thank you

@Saumyachitransh01 Saumyachitransh01 deleted the branchNew branch January 5, 2021 12:38
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.

Date and Time: Standardize it across system
3 participants