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

Show "Publish: Immediately" for new drafts by inferring floating date #9967

Merged
merged 3 commits into from Oct 2, 2018

Conversation

Projects
None yet
5 participants
@kadamwhite
Contributor

kadamwhite commented Sep 17, 2018

Description

This addresses the post date label issue described in #7195 with an alternative to the previously-proposed solution in PR #8395.

Rather than forcing a new property into the API to work around the nuances of how dates are stored in the database, we can use what @earnjam and @TimothyBJacobs proposed in https://core.trac.wordpress.org/ticket/39953 and infer that a post's date is "floating" (i.e. that it should be published immediately) by comparing the date modified to the listed publish date. In that Trac thread @TimothyBJacobs explained,

If the date and the modified date are the same, then we're still working on the post because those are both updated at every write. As mentioned we don't have an actual created_at date. This would save us having to add the new field. We couldn't figure out a time when those two dates wouldn't match, and in testing it looked like it would work to fulfill Gutenberg's needs.

Note that this works in Gutenberg because on save, we only send back values which have been edited. To fully address this issue throughout WordPress, changes will be needed in Trac to ensure that we apply the same logic when preparing posts to be saved to the database.

How has this been tested?

I created this patch with @earnjam at the WCNYC contributor day, and I have tested it by creating several posts to ensure various combinations of draft and auto-draft correctly reflect manually-specified dates and the Immediately placeholder. To reproduce these tests,

  1. Create a new post and look at the date label. It should display "Immediately."
  2. Update the title (or any other non-date field) and save a draft. Post should still display Publish: "Immediately."
  3. Test publish vs manually-specify date:
  • To test that date is set properly when publishing, publish the post and ensure the date that is displayed on the front-end and when reloading the post in the editor reflects the time at which the post was published.
  • To test that the date is set properly when manually specifying a date, click "Immediately" and set a manual date at which the post should be published. Reload the page and ensure the provided date still displays.

Screenshots

image

Types of changes

Bug fix for PostScheduleLabel component.
Adds unit tests for PostScheduleLabel component.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.

tofumatt added a commit that referenced this pull request Sep 26, 2018

fix: Show "Publish Immediately"
* Close #7195
* Close #8395
* Close #9030
* Close #9967
* Fix #10182
Add isEditedPostDateFloating selector
Move logic to determine whether a post should be considered to have a
floating date from the PostScheduleLabel component into a new selector
isEditedPostDateFloating.
@kadamwhite

This comment has been minimized.

Show comment
Hide comment
@kadamwhite

kadamwhite Sep 27, 2018

Contributor

@youknowriad I have added a commit to introduce a isEditedPostDateFloating selector, which produces a value to be consumed within the component instead of relying on the component code to apply the date logic.

Contributor

kadamwhite commented Sep 27, 2018

@youknowriad I have added a commit to introduce a isEditedPostDateFloating selector, which produces a value to be consumed within the component instead of relying on the component code to apply the date logic.

@youknowriad

Thanks for the update, Code looks good, I'll just do some testing a bit after (unless someone beats me to it) and approve.

Show outdated Hide outdated packages/editor/src/components/post-schedule/label.js Outdated
@kadamwhite

This comment has been minimized.

Show comment
Hide comment
@kadamwhite

kadamwhite Sep 27, 2018

Contributor

Good point about the property naming, I have made that change 👍

Contributor

kadamwhite commented Sep 27, 2018

Good point about the property naming, I have made that change 👍

*
* @return {boolean} Whether the edited post has a floating date value.
*/
export function isEditedPostDateFloating( state ) {

This comment has been minimized.

@youknowriad

youknowriad Sep 27, 2018

Contributor

I expect this new selector to generate some changes in the docs if you run npm run docs:build. Can you commit these changes?

@youknowriad

youknowriad Sep 27, 2018

Contributor

I expect this new selector to generate some changes in the docs if you run npm run docs:build. Can you commit these changes?

This comment has been minimized.

@aduth

aduth Oct 3, 2018

Member

I expect this new selector to generate some changes in the docs if you run npm run docs:build. Can you commit these changes?

This was never done and there are local changes to commit on master after running the command.

Please review #10234

@aduth

aduth Oct 3, 2018

Member

I expect this new selector to generate some changes in the docs if you run npm run docs:build. Can you commit these changes?

This was never done and there are local changes to commit on master after running the command.

Please review #10234

This comment has been minimized.

@kadamwhite

kadamwhite Oct 4, 2018

Contributor

😬 my bad, @aduth , hadn't had time to circle back to this and had missed that piece of feedback. I'll know for next time.

@kadamwhite

kadamwhite Oct 4, 2018

Contributor

😬 my bad, @aduth , hadn't had time to circle back to this and had missed that piece of feedback. I'll know for next time.

This comment has been minimized.

@aduth

aduth Oct 4, 2018

Member

No worries @kadamwhite

@aduth

aduth Oct 4, 2018

Member

No worries @kadamwhite

@youknowriad

LGTM 👍

@youknowriad youknowriad added this to the 4.0 milestone Sep 27, 2018

@youknowriad youknowriad merged commit af53caa into master Oct 2, 2018

2 checks passed

codecov/project 48.96% (+0.2%) compared to 2b42db5
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@youknowriad youknowriad deleted the fix/7195-infer-publish-immediately branch Oct 2, 2018

@mtias

This comment has been minimized.

Show comment
Hide comment
@mtias

mtias Oct 9, 2018

Contributor

Thanks for the work here @kadamwhite.

Contributor

mtias commented Oct 9, 2018

Thanks for the work here @kadamwhite.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment