Clear Publish Date Button #20914
Clear Publish Date Button #20914
Conversation
|
Size Change: -2.94 kB (0%) Total Size: 822 kB
ℹ️ View Unchanged
|
earnjam
force-pushed
the
try/clear-publish-date
branch
from
21f14ad to
7f4459d
Compare
earnjam
added
Needs Design Feedback
|
The failing test is just related to |
earnjam
force-pushed
the
try/clear-publish-date
branch
from
7f4459d to
b7e8b4c
Compare
|
I think from a design perspective this should be a link not a button. That would visually make more sense right now. I realise a while ago a button was recommended but we're going back to using links for a lot of things, so this feels right. |
That would definitely take some of the weight of it away. The button does feel a bit heavy for the space at the moment. I'll get the PR updated with a link. |
|
Switched it over to a link, but there is still a bug I discovered that needs to be worked out. Focus is set on the day field in the datepicker when it is opened, and then the date gets updated |
|
Looking good, @earnjam. I've got just a few design changes. Current iteration in PR:
Proposed design improvements:
|
|
Thanks @mapk! Updated with the suggested changes.
|
earnjam
requested review from
ajitbohra,
nerrad,
ntwb and
talldan
as code owners
|
Added an e2e test |
|
I think this action should be added to the calendar popover itself so the flow of setting / unsetting a date is more consistent. |
I agree in principle, but if you read the conversation from #12048, there are a lot of a11y issues to consider for the calendar popover that make adding save/cancel buttons a complicated problem to solve. This was the short term solution proposed while a more robust design effort could be made there. |
|
@youknowriad or @jorgefilipecosta can we get a technical review on this before merging? |
| @@ -33,6 +33,9 @@ export default compose( [ | |||
| withSelect( ( select ) => { | |||
| return { | |||
| date: select( 'core/editor' ).getEditedPostAttribute( 'date' ), | |||
| modified: select( 'core/editor' ).getEditedPostAttribute( | |||
| 'modified' | |||
There was a problem hiding this comment.
what is this "modified" attribute? and why it's being used here? Also, it seems that this change is unrelated with the root issue of the PR (unschedule link)?
There was a problem hiding this comment.
modified is the timestamp the post was last modified. modified === date is one of the ways we determine that the post is considered to have a floating date.
If you pass a null value for currentDate to the DateTimePicker then it uses the current date to pre-populate the input fields. Then onBlur it will save the current value of the input as date, effectively eliminating the floating date we just set. It immediately closes and uses the current time no matter what you click inside it.
The REST API expects null in order to make a date floating, so that's why we're using that for the date.
|
A lot of considerations from that ticket, for sure, but I still think a "reset" added to the date popover is more intuitive and helpful for the component's behaviour for others that might make use of it. |
|
I have mocked up what that reset might look like here:
It's super simple but should work. I don't think we need it to be 'red' as it's not deleting just resetting. |
earnjam
force-pushed
the
try/clear-publish-date
branch
from
e1048d6 to
b6b3d23
Compare
|
I moved put a I also rebased A few things to note here:
The So the only options I saw to solve this were:
The latter is a much easier approach for now, short of a bigger effort to improve component as a whole.
|
mapk
added
Needs Technical Feedback
|
I tested this and was able to schedule a post, reset that publish date and then publish the post immediately. It works as expected. Let's get some technical feedback before merging. cc @youknowriad |
|
Great work here
That's the only thing bothering me in this PR. I feel it's a bit of a hack, and I wonder if there's a small fix like: on focus out, if the value was "untouched" (a local boolean to keep track of when the value really changes from the one used as a fallback) trigger the onChange, otherwise don't. cc @ItsJonQ in case you have an idea about how to represent the "current" time (null value) in the TimePicker component properly. |
Good call. I looked into Seems to work great and also solves the issue of accidentally setting the date on a new post just by tabbing past the month field. |
|
Thanks for this improvement! |
|
Really great work! I'm so excited about this one. 🎉 |
I've been noticing a trend of... "how do we handle value parsing + resetting for controls?" lately 😂 . Some from PRs, but mostly from my work on padding controls. I'm starting to get a sense of a "standardized" pattern of doing things. However, applying it to the DatePicker. That I'm unsure of (for now) |
Description
This PR adds an
Unschedulebutton to the sidebar below the Publish Date. The button only shows when:It allows the user to clear the selected publish date and returns it to a floating date, displaying 'Immediately'.
This is the simplest implementation of the mock-ups from the ticket by @karmatosed .
Fixes #12048
How has this been tested?
Screenshots
Types of changes
New feature (non-breaking change which adds functionality)
Checklist: