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

Remove moment from the public API of the date module #11418

Merged
merged 2 commits into from Nov 6, 2018

Conversation

Projects
None yet
3 participants
@youknowriad
Contributor

youknowriad commented Nov 2, 2018

I'm doing so by adding a new function in the data module isInTheFuture to check whether a given WP date is considered in the future or not.

Testing instructions

  • Make sure scheduling still works as expected (especially if the WP timezone is different from the browser's timezone).

@youknowriad youknowriad added this to the 4.3 milestone Nov 2, 2018

@youknowriad youknowriad self-assigned this Nov 2, 2018

@youknowriad youknowriad requested a review from WordPress/gutenberg-core Nov 2, 2018

@tofumatt

Looks good to me; mostly curious about "sdays" and have some minor tweak suggestions.

Show resolved Hide resolved packages/date/CHANGELOG.md
Show resolved Hide resolved packages/date/src/index.js
@@ -1497,10 +1496,12 @@ describe( 'selectors', () => {
describe( 'isEditedPostBeingScheduled', () => {
it( 'should return true for posts with a future date', () => {
const time = Date.now() + ( 1000 * 3600 * 24 * 7 ); // sdays in the future

This comment has been minimized.

@tofumatt

tofumatt Nov 2, 2018

Member

What does "sdays" mean here?

This comment has been minimized.

@youknowriad

youknowriad Nov 2, 2018

Contributor

It means "7 days", it's a new language invented by my broken keyboard :)

@@ -300,7 +300,7 @@ export function isCurrentPostPublished( state ) {
const post = getCurrentPost( state );
return [ 'publish', 'private' ].indexOf( post.status ) !== -1 ||
( post.status === 'future' && moment( post.date ).isBefore( moment() ) );
( post.status === 'future' && ! isInTheFuture( post.date, 1 ) );

This comment has been minimized.

@aduth

aduth Nov 2, 2018

Member

Will a future maintainer understand what the second argument it is by glancing at it? It confuses me just now looking at it.

This comment has been minimized.

@aduth

aduth Nov 2, 2018

Member

Can we prepare this ahead of time? If it's an ISO-8601 string, could we just do new Date( Number( new Date( post.date ) ) + MINUTE_IN_MS )

@youknowriad youknowriad force-pushed the remove/moment-from-public-api-data branch from 8e9b7f3 to 03102d4 Nov 5, 2018

@youknowriad youknowriad merged commit a1b647d into master Nov 6, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@youknowriad youknowriad deleted the remove/moment-from-public-api-data branch Nov 6, 2018

grey-rsi pushed a commit to OnTheGoSystems/gutenberg that referenced this pull request Nov 22, 2018

Remove moment from the public API of the date module (WordPress#11418)
* Remove moment from the public API of the date module

* Remove the offset argument and leave the computation to the caller
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment