Chrome: Adding the post date picker to schedule posts #841

youknowriad · 2017-05-19T11:53:25Z

This PR adds the post data picker to schedule a post.

Using react-datepicker. Are we ok with that? should we create our own date picker
Using moment
I'm using the post date GMT field to ease working with the dates. I know there are some discussions around this in Core. Any information I should know about here?

Not done yet

I'm using the default styling from react-datepicker. This needs to be tweaked (@joen make me pretty 😄)
We need to set the locale to the moment object globally moment.locale( locale )(cc @aduth). Should we do this in the i18n module assuming moment is used globally in WordPress?
The time picker is not localized too, it always uses a AM/PM selector.

jasmussen · 2017-05-19T12:33:35Z

This seems good to me, and I'm okay with having to do a little CSS'ing to get it up to snuff ;)

However we will encounter some internationalization things. This could already be adressed and I haven't checked so forgive me if I'm wrong. But I immediately notice the AM/PM toggle, which as a european I find mostly useless. So I'm just checking to make sure, have we a plan for hooking into the standards that WordPress' options table gives us here, including date format?

Also, can we ease the workload by using some of the code from Calypso? There's no reason to reinvent the wheel since they are both GPL.

aduth · 2017-05-19T12:43:45Z

We should pay some mind to this ticket: https://core.trac.wordpress.org/ticket/39222

Specifically on the proposed abstraction that doesn't tie us to Moment specifically. Dunno how we'd want to interop with that patch though.

youknowriad · 2017-05-19T13:42:09Z

If we want to move forward with this branch, maybe we should copy this patch temporarily?

aduth · 2017-05-19T14:30:16Z

I'd be fine with that, but the specifics of copying are a bit fuzzy to me.

youknowriad · 2017-05-22T11:51:25Z

I've copied and adapted the patch from https://core.trac.wordpress.org/ticket/39222 to match Gutenberg "guidelines".
I also had to update the code, especially the calls to utcOffset to set the second parameter to true. It wasn't working as I expected it to work if I didn't do that.
I've also exposed the date settings (timezone, formats) to be able to use them in the PostSchedule component
I show/hide the AP/PM selector depending on the timezone.

Thoughts on these updates?

jasmussen · 2017-05-22T11:54:54Z

I like it!

Can we pre-fill the time with the current time, so it isn't empty until you click a date?

youknowriad · 2017-05-22T12:14:36Z

@aduth do you have an idea how to fix this unit test failure?

aduth · 2017-05-22T13:16:45Z

088fd8f should clear it up. I don't think we'd intended to allow importing .css files, though the Webpack configuration is set up to match them. From the error it appeared the CSS was being interpreted as JavaScript. Instead of trying to accommodate CSS imports from the component, I moved the import into the component's SASS stylesheet.

youknowriad · 2017-05-23T09:38:24Z

I'd use some technical 👀 on this. Thanks :)

aduth · 2017-05-23T15:37:17Z

editor/sidebar/post-schedule/style.scss

+.wp-core-ui .editor-post-schedule__clock-am-button.is-toggled,
+.wp-core-ui .editor-post-schedule__clock-pm-button.is-toggled {
+	background: $light-gray-300;
+  border-color: $dark-gray-100;


Mixed spacing (needs tab).

aduth · 2017-05-23T15:50:49Z

editor/sidebar/post-schedule/index.js

+		const handleChange = ( newDate ) => {
+			onUpdateDate( newDate.format( 'YYYY-MM-DDTHH:mm:ss' ) );
+		};
+		const is12HourTime = settings.formats.time.indexOf( 'a' ) !== -1 || settings.formats.time.indexOf( 'A' ) !== -1;


Can be inaccurate because it's possible to escape characters in the format. In other words, 'H:i:s \a\w\e\s\o\m\e' is not twelve hour time, but this logic would treat it as though it were.

Also, it could be simpler to force to lower/upper and test index of the one instance of 'a'/'A'.

Accounting for escaping could be tricky, particularly since JavaScript doesn't support lookbehind regular expression patterns. Dunno if there's a better way, but "flipping" the string and doing a look ahead is one option:

/a(?!\\)/i.test( 'h:i:s a'.split( '' ).reverse().join( '' ) ) // true /a(?!\\)/i.test( 'h:i:s A'.split( '' ).reverse().join( '' ) ) // true /a(?!\\)/i.test( 'H:i:s \\a\\w\\e\\s\\o\\m\\e'.split( '' ).reverse().join( '' ) ) // false

Even this solution is not perfect because this \\\\\a is a 12 hour time but the test returns false

I guess this is the right test /a(?!\\)/i.test( '\\\\\\\\a'.replace(/\\\\/g).split( '' ).reverse().join( '' ) )

youknowriad · 2017-05-24T15:57:56Z

Rebased this PR. I'm planning to merge this tomorrow if no more raised concerns.

mcsf · 2020-09-29T13:20:27Z

editor/sidebar/post-schedule/index.js

+		// To know if the current timezone is a 12 hour time with look for "a" in the time format
+		// We also make sure this a is not escaped by a "/"
+		const is12HourTime = /a(?!\\)/i.test(
+			settings.formats.time
+				.toLowerCase() // Test only the lower case a
+				.replace( /\\\\/g, '' ) // Replace "//" with empty strings
+				.split( '' ).reverse().join( '' ) // Reverse the string and test for "a" not followed by a slash
+		);


I know how old this PR is :), but today we have multiple occurrences of this snippet in the codebase, so it's worth asking:

Why reverse the string and not use a non-capturing group like /(?:[^\\])a/?

If the RegExp already has the i flag, why pipe through .toLowerCase?

youknowriad added the General Interface Parts of the UI which don't fall neatly under other labels. label May 19, 2017

youknowriad self-assigned this May 19, 2017

youknowriad requested review from jasmussen and aduth May 19, 2017 11:53

youknowriad force-pushed the add/post-datepicker branch from 088fd8f to b394b86 Compare May 23, 2017 09:36

aduth reviewed May 23, 2017

View reviewed changes

youknowriad and others added 8 commits May 24, 2017 16:57

Chrome: Adding the post date picker to schedule posts

eb8b1d9

Post Schedule: Include wp.date script and localize the datepicker

3568362

Post Schedule: Depending on the timezone show/hide the AM/PM selector

9625759

WP Date: Fix PHPCS test

45e1913

WP Date: Fix JS unit tests

8189cb7

Post Schedule: Prefill the post schedule modal

27fd6c9

Move date picker styles into PostSchedule stylesheet

cce5fdb

Chrome: Check more accurately the 12 hour time

b7e0954

youknowriad force-pushed the add/post-datepicker branch from 995bb12 to b7e0954 Compare May 24, 2017 15:57

youknowriad merged commit 256badd into master May 25, 2017

youknowriad deleted the add/post-datepicker branch May 25, 2017 08:29

youknowriad mentioned this pull request May 25, 2017

Sidebar: Status & Visibility panel #852

Closed

nylen mentioned this pull request May 26, 2017

Fix whether moment.js is minified or not based on SCRIPT_DEBUG #918

Merged

aduth mentioned this pull request Dec 9, 2019

Make dateI18n returns be affected by gmt parameter #18982

Merged

5 tasks

mcsf reviewed Sep 29, 2020

View reviewed changes

This was referenced Dec 6, 2022

[Snyk] Security upgrade simple-git from 2.48.0 to 3.15.0 MaxMood96/gutenberg#80

Open

[Snyk] Security upgrade simple-git from 2.35.0 to 3.15.0 MaxMood96/gutenberg#81

Open

tyxla mentioned this pull request Jun 16, 2023

Use moment-timezone-data-webpack-plugin to optimize timezones shipped in wp/date #51519

Merged

MaxMood96 mentioned this pull request Jan 13, 2024

[Snyk] Security upgrade simple-git from 3.5.0 to 3.15.0 MaxMood96/gutenberg#135

Open

naiba4 mentioned this pull request Apr 23, 2024

[Snyk] Security upgrade simple-git from 3.5.0 to 3.15.0 naiba4/gutenberg#56

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Chrome: Adding the post date picker to schedule posts #841

Chrome: Adding the post date picker to schedule posts #841

youknowriad commented May 19, 2017 •

edited

Loading

jasmussen commented May 19, 2017

aduth commented May 19, 2017

youknowriad commented May 19, 2017 •

edited

Loading

aduth commented May 19, 2017

youknowriad commented May 22, 2017

jasmussen commented May 22, 2017

youknowriad commented May 22, 2017

aduth commented May 22, 2017

youknowriad commented May 23, 2017

aduth May 23, 2017

aduth May 23, 2017

youknowriad May 24, 2017

youknowriad May 24, 2017

youknowriad commented May 24, 2017

mcsf Sep 29, 2020

Chrome: Adding the post date picker to schedule posts #841

Chrome: Adding the post date picker to schedule posts #841

Conversation

youknowriad commented May 19, 2017 • edited Loading

jasmussen commented May 19, 2017

aduth commented May 19, 2017

youknowriad commented May 19, 2017 • edited Loading

aduth commented May 19, 2017

youknowriad commented May 22, 2017

jasmussen commented May 22, 2017

youknowriad commented May 22, 2017

aduth commented May 22, 2017

youknowriad commented May 23, 2017

aduth May 23, 2017

Choose a reason for hiding this comment

aduth May 23, 2017

Choose a reason for hiding this comment

youknowriad May 24, 2017

Choose a reason for hiding this comment

youknowriad May 24, 2017

Choose a reason for hiding this comment

youknowriad commented May 24, 2017

mcsf Sep 29, 2020

Choose a reason for hiding this comment

youknowriad commented May 19, 2017 •

edited

Loading

youknowriad commented May 19, 2017 •

edited

Loading