Add comment_status field to quick edit
#64370
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
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. |
oandregal
added
[Type] Enhancement
oandregal
requested review from
youknowriad,
ellatrix,
jorgefilipecosta and
ntsekouras
oandregal
commented
Aug 8, 2024
youknowriad
reviewed
Aug 8, 2024
packages/dataviews/src/types.ts
Outdated
| /** | ||
| * Optional config for editing the field. | ||
| */ | ||
| editAs?: 'radio'; |
There was a problem hiding this comment.
I do think we need a config like that. Maybe I would name it "control".
I think this also means that like "field types" we need a folder of "controls" and we already have three of them (NumberControl, TextControl and SelectControl). I believe these should be treated similarly to RadioControl.
There was a problem hiding this comment.
Given there's a Edit prop to provide a custom edit function, how about editControl so it's connected by the prefix?
My thinking is that "controls" seem too open-ended: it seems you can list any control and it'll be used. I see it more as a "edit configuration" if that makes sense.
There was a problem hiding this comment.
The question I'm wondering though is whether this should be part of the field definition or the form object.
There was a problem hiding this comment.
It is "edit" configuration yes, but I don't like the name "editAs" personally. I think the question also is whether we see folks registering new "controls" without necessarily changing the "type" of the field. And the answer for me seems "yes" given that the "type" controls more than just "edit", I want to use the "integer" sorting/validation... but I want to build a custom control for my field.
There was a problem hiding this comment.
How about changing the Edit signature to also support a predefined set of controls?
So it would be Edit: 'radio'.
There was a problem hiding this comment.
How about changing the Edit signature to also support a predefined set of controls?
So it would be Edit: 'radio'.
I like that but I would still prefer "control" over "edit".
There was a problem hiding this comment.
I reworked this to overload Edit, this was my alternative option as well.
youknowriad
reviewed
Aug 8, 2024
| @@ -49,6 +53,18 @@ function Edit< Item >( { | |||
| [ id, onChange ] | |||
| ); | |||
|
|
|||
| if ( field.elements && field.editAs === 'radio' ) { | |||
There was a problem hiding this comment.
I think we need to remove all these conditions and have a folder for control and something like:
const Control = getFieldControl( field )
<Control />
|
Size Change: +472 B (+0.03%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
ntsekouras
reviewed
Aug 8, 2024
oandregal
force-pushed
the
add/quick-edit-discussion
branch
from
9e71d6f
to
5cb2991
Compare
|
I pushed an initial version that introduces controls (only |
youknowriad
reviewed
Aug 9, 2024
| @@ -0,0 +1,43 @@ | |||
| /** | |||
There was a problem hiding this comment.
Personally, I would move the "text", "radio", "select" and "integer control to. src/controls or something like that. But can be done in a separate PR.
youknowriad
reviewed
Aug 9, 2024
| /** | ||
| * An abstract interface for Field based on the field type. | ||
| */ | ||
| export type FieldTypeDefinition< Item > = { |
There was a problem hiding this comment.
👍
One thing I'm wondering is that "field types" shouldn't depend on "Item" they should work for any item value of the defined type. So I think we have somethings wrong in the abstraction. sort, isValid and Edit should probably not receive items at all and just receive values. But it's also something that should be solved in a separate PR.
|
While testing this, I noticed that something regressed recently when it comes to long columns (long titles) 
cc @jameskoster if you're aware of a related change. |
youknowriad
approved these changes
Aug 9, 2024
Ok, I'll merge as it is then. I'm going to set this to auto-merge and focus a bit on clearing my queue — I don't expect to come back to these follow-ups today. |
getdave
pushed a commit
that referenced
this pull request
Aug 14, 2024
Co-authored-by: oandregal <oandregal@git.wordpress.org> Co-authored-by: youknowriad <youknowriad@git.wordpress.org> Co-authored-by: ntsekouras <ntsekouras@git.wordpress.org>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Part of #55101 #59745
What?
Adds
comment_status(Discussion) field to quick edit.How?
textfield with two elements (open & close). That's how the field is added as well.editAsfunction that enables the consumer to configure the Edit function.Testing Instructions