-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Add status
to quick edit
#64398
Add status
to quick edit
#64398
Conversation
I was playing a bit with status, and I think we need to have a custom edit function for this one: there's too much custom logic to have built-in in DataForm. |
Size Change: +198 B (+0.01%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
elements: STATUSES, | ||
render: PostStatusField, | ||
Edit: 'radio', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When we edit the status in the editor, we do some extra modifications See
gutenberg/packages/editor/src/components/post-status/index.js
Lines 150 to 164 in b12e897
const handleStatus = ( value ) => { | |
let newDate = date; | |
let newPassword = password; | |
if ( status === 'future' && new Date( date ) > new Date() ) { | |
newDate = null; | |
} | |
if ( value === 'private' && password ) { | |
newPassword = ''; | |
} | |
updatePost( { | |
status: value, | |
date: newDate, | |
password: newPassword, | |
} ); | |
}; |
I wonder how we should deal with that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And that's not all of it. We also show the publish date when a user selects future
. Probably it makes sense to export privately the control we use in the editor
package and reuse it here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And that's not all of it. We also show the publish date when a user selects future.
This for me is a weird interactions and I know designers feel like it's a good thing but I have some doubts about it personally :)
For the handleStatus
function, we can either absorb it as an API or move it to the onChange
handler of the DataForm
instance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding or not more components to the status field can be a follow-up. Were we to do that, we could create a custom Edit
function for status
.
395115f
to
6f9dd3e
Compare
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
1babdc0
to
f234031
Compare
@youknowriad @ntsekouras I think we can land this one. Thoughts? |
const fields = useMemo( | ||
() => | ||
_fields?.map( ( field ) => { | ||
if ( field.id === 'status' ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like this personally but I see why you did that. I wonder if this is something that is common: A way to make an "element" readonly basically. If we should absorb it into the API or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm ok shipping this but can we add add an inline comment to explain why we do this and that we should ideally reuse the fields untouched here (so it doesn't become a common practice).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it seems something that should be part of the field and not check specific ids here. Definitely comment about it if we land as is..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the idea of "readonly" elements, but I'm not sure we should make any changes to the API just yet. What I'm seeing here is that parts of the field need to be different depending on whether it's used in the view or in the edit context. For example, the render function for date is the same: in the edit context I'd have it without the "prefix" (Modified/Scheduled/etc.).
Kind of unrelated, but I noticed that when switching the status the "date" field switches from "modified" to "published" ... I think the "date" field is confusing and we should probably not use it in "quick edit", instead we should just use the "publish date" or something more explicit instead of combining multiple fields. |
The other thing I noticed is that when you set something to "publish" for instance, there's no publish flow that triggers, you just click "save" and your page is published. Probably it's fine though (it's the same in WP-Admin classic) |
Yeah, we chatted with André about the nuances with status and publish date and since it's the same with wp-admin it could be okay I guess - at least for start. |
The date field uses the The confusion comes from the prefix we add to the date value, in my view: Modified for draft/private statuses, Scheduled for future, Published for publish. |
It's fine as it is given that it behaves the same as the "wp-admin quick edit" and it's experimental. I'd be good to get user feedback on this one and there's probably room for improvement as well. cc @jameskoster My main concern is that changing date and status can make posts published/unpublished. When you use the editor, you have some guardrails (the pre-publish panel) but you may or may not in the site editor. This is related to what I shared when we switched to rely on the global "save entity" flow. I believe we should
|
Examples of some weird interactions with date & status in the editor:
Gravacao.do.ecra.2024-08-12.as.11.30.46.mov
Gravacao.do.ecra.2024-08-12.as.11.34.28.mov
Gravacao.do.ecra.2024-08-12.as.11.35.14.mov |
@oandregal Is it possible that this PR added descriptions to the status filter UI: They weren't there before: |
Oh, certainly. By adding the "description" to the statuses, it'll be used everywhere (filters + quick edit). I read it's undesired behavior? Sorry about that. I can explore how to make the elements for filters & edit different. They already are (see this conversation), so we may want to consolidate in the API. One way is the following:
For this case, |
PR at #64674 |
I like this. For those only using the DataViews component, having |
Part of #55101
What?
Add the
status
field to the quick edit form. Also reorder the fields to mimick the document inspector order.How?
getValue
86d1571Testing Instructions
Modify the status and verify that it works properly.