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.

Sign up for GitHub

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

Jump to bottom

EZEE-2481: Time picker bases on system time ignoring time zone in user settings #893

Merged
merged 4 commits into from
Mar 19, 2019
Merged

EZEE-2481: Time picker bases on system time ignoring time zone in user settings #893

merged 4 commits into from
Mar 19, 2019

Conversation

tischsoic
Copy link
Contributor

@tischsoic tischsoic commented Mar 12, 2019 •
edited
Loading

Question Answer
Tickets https://jira.ez.no/browse/EZEE-2481, https://jira.ez.no/browse/EZEE-2480
Bug fix? yes
New feature? no
BC breaks? no
Tests pass? yes
Doc needed? no
License GPL-2.0

Checklist:

  • Coding standards ($ composer fix-cs)
  • Ready for Code Review

@tischsoic tischsoic added the Bug label Mar 12, 2019
@tischsoic tischsoic self-assigned this Mar 12, 2019
@tischsoic tischsoic requested a review from dew326 March 12, 2019 08:23
@tischsoic tischsoic changed the title EZEE-2480: Allow user to select date in ezdatetime in their timezone EZEE-2480: Time picker bases on system time ignoring time zone in user settings Mar 12, 2019
@tischsoic tischsoic changed the title EZEE-2480: Time picker bases on system time ignoring time zone in user settings EZEE-2481: Time picker bases on system time ignoring time zone in user settings Mar 12, 2019
@tischsoic
Copy link
Contributor Author

tischsoic commented Mar 13, 2019 •
edited
Loading

As discussed with @SylvainGuittard and @supriyabhargava, eztime field should not have timezone. For all users, whatever timezone they have, it should show the same value.

@SylvainGuittard mentioned "Default value: current time" option in ezdatetime in connected PR in different repository: https://github.com/ezsystems/date-based-publisher/pull/92
I checked how it works and value is pre-filled by backend during creation and on the frontend it is displayed with user timezone, so it should be OK.

I didn't change function responsible for formatting date. There is a problem with ezdatetime and eztime with seconds - my default date format in Demo does not include seconds, hence when date is formatted with format set in user settings it won't display seconds. This problem will also be present here: https://jira.ez.no/browse/EZP-30263 .
This problem was also discussed with @SylvainGuittard.

format has been changed: #893 (comment)

tischsoic
tischsoic commented Mar 13, 2019
View reviewed changes
src/bundle/Resources/public/js/scripts/fieldType/ezdatetime.js
@@ -91,13 +94,24 @@
const sourceInput = field.querySelector(SELECTOR_INPUT);
const flatPickrInput = field.querySelector(SELECTOR_FLATPICKR_INPUT);
const btnClear = field.querySelector('.ez-data-source__btn--clear-input');
const defaultDate = sourceInput.value ? new Date(sourceInput.value * 1000) : null;
const secondsEnabled = sourceInput.dataset.seconds === '1';
const formatDate = secondsEnabled ? (date) => date.toLocaleString() : (date) => eZ.helpers.timezone.formatDate(date);
Copy link
Contributor Author

@tischsoic tischsoic Mar 13, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Note: As agreed with @SylvainGuittard, if seconds are enabled we fall back to date.toLocaleString() because there it high probability that the date format from user settings does not include seconds. Otherwise we use the date format from user settings.

@tischsoic
Copy link
Contributor Author

Changed to use only convertDateToTimezone - no moment, or tz - ping @dew326.

@katarzynazawada katarzynazawada self-assigned this Mar 14, 2019
@tischsoic tischsoic force-pushed the EZEE-2480-preselected-time-in-date-picker-is-not-current-time-from-users-timezone branch from 8b38ddb to 556a7e6 Compare March 15, 2019 09:54
@tischsoic tischsoic changed the title EZEE-2481: Time picker bases on system time ignoring time zone in user settings [WIP] EZEE-2481: Time picker bases on system time ignoring time zone in user settings Mar 15, 2019
@tischsoic
Copy link
Contributor Author

tischsoic commented Mar 15, 2019 •
edited
Loading

WIP - during fixing - Behat tests fail.

@tischsoic tischsoic force-pushed the EZEE-2480-preselected-time-in-date-picker-is-not-current-time-from-users-timezone branch from 568557b to 556a7e6 Compare March 18, 2019 08:11
@tischsoic tischsoic force-pushed the EZEE-2480-preselected-time-in-date-picker-is-not-current-time-from-users-timezone branch from 8e223ba to d43866b Compare March 19, 2019 08:26
@tischsoic tischsoic changed the title [WIP] EZEE-2481: Time picker bases on system time ignoring time zone in user settings EZEE-2481: Time picker bases on system time ignoring time zone in user settings Mar 19, 2019
@tischsoic
Copy link
Contributor Author

Behat tests fixed by @mnocon 🙂

@lserwatka lserwatka merged commit ad85331 into 1.4 Mar 19, 2019
@lserwatka lserwatka deleted the EZEE-2480-preselected-time-in-date-picker-is-not-current-time-from-users-timezone branch March 19, 2019 14:29
@lserwatka
Copy link
Member

Could you merge it up?

@tischsoic
Copy link
Contributor Author

Merged up:

@tischsoic tischsoic mentioned this pull request Mar 22, 2019
Merged
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@katarzynazawada katarzynazawada katarzynazawada approved these changes

@dew326 dew326 dew326 approved these changes

Assignees

@tischsoic tischsoic

@katarzynazawada katarzynazawada

Labels
Bug QA approved
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@tischsoic @lserwatka @dew326 @katarzynazawada