QuickEdit: Add Parent field #66527
Merged
QuickEdit: Add Parent field #66527
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
|
Size Change: +1.09 kB (+0.06%) Total Size: 1.81 MB
ℹ️ View Unchanged
|
gigitux
added
[Feature] DataViews
|
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. |
11 tasks
oandregal
added
[Type] Experimental
Thanks for adding the labels! The current setup requires at least one label to be of the Also, for the PR title: we use this to auto-generate the changelog that is published as part of the release. I understand the package name ( |
Thanks!
Sounds great! Thanks for fixing both 🙇 |
oandregal
reviewed
Oct 29, 2024
| enableSorting: false, | ||
| }; | ||
|
|
||
| /** |
There was a problem hiding this comment.
Thanks for documenting it (README) :)
oandregal
reviewed
Oct 29, 2024
| getValue: ( { item } ) => item.parent, | ||
| Edit: ParentEdit, | ||
| render: ParentView, | ||
| enableSorting: false, |
There was a problem hiding this comment.
Why disabling sorting? The endpoint we take the data from supports sorting by parent.
oandregal
reviewed
Oct 29, 2024
| return <>{ getTitleWithFallbackName( parent ) }</>; | ||
| } | ||
|
|
||
| return <>{ __( 'None' ) }</>; |
There was a problem hiding this comment.
This works for me: we could land as it is and then gather feedback from folks. Though I wonder if there's a better default for this because None could be an actual title, so it may be confusing. Perhaps — like wp-admin uses in some places?
There was a problem hiding this comment.
I would proceed in this way.
None is used in the Document Inspector panel:
There was a problem hiding this comment.
Oh, nice, it makes more sense then.
oandregal
reviewed
Oct 29, 2024
| */ | ||
| import type { BasePost } from '../../types'; | ||
|
|
||
| export function getTitleWithFallbackName( post: BasePost ) { |
There was a problem hiding this comment.
There's a getItemTitle utilility in actions. Can we reuse that one?
There was a problem hiding this comment.
Interestingly, it appears that every usage of decodeEntities in the fields package can be replaced by just calling that utility?
In some situations, we are running it twice:
decodeEntities( decodeEntities( '...' ) );There was a problem hiding this comment.
What do you think about?
export function getTitleWithFallbackName( post: BasePost ) {
const title = getTitle(post)
return title.length > 0 ? title : `#${ post?.id } (${ __( 'no title' ) })`;
There was a problem hiding this comment.
I guess my point was that we didn't need getTitleWithFallbackName and we could reuse getItemTitle, unless I'm missing something?
There was a problem hiding this comment.
This function is simply a way to centralize the fallback return value. Otherwise, we need to implement it multiple times.
const title = getTitle(post)
title.length > 0 ? title : `#${ post?.id } (${ __( 'no title' ) })`;
In the specific, we should do repeat this check three times:
There was a problem hiding this comment.
I guess what I don't understand is why we have different ways to get the title from a Post across the app. Anyway, don't feel this PR should be blocked by this comment. This can be looked at separately.
oandregal
reviewed
Oct 29, 2024
| const flatTermsWithParentAndChildren = flatTerms.map( ( term ) => { | ||
| return { | ||
| children: [], | ||
| ...term, |
There was a problem hiding this comment.
Interesting how using TypeScript here forced us to declare the Term API (parent obligatory), making the prop redundant here compared to the original that used:
{
children: [],
parent: null,
...term,
}
oandregal
reviewed
Oct 29, 2024
| label={ __( 'Parent' ) } | ||
| help={ __( 'Choose a parent page.' ) } | ||
| value={ parentPostId?.toString() } | ||
| options={ parentOptions.map( ( option ) => ( { |
There was a problem hiding this comment.
It doesn't seem parentOptions is used anywhere else, so can we make this part of the useMemo above? Otherwise, this is creating a new set of options in every render.
oandregal
reviewed
Oct 29, 2024
| } ) { | ||
| const [ fieldValue, setFieldValue ] = useState< null | string >( null ); | ||
|
|
||
| const pageId = data.parent; |
There was a problem hiding this comment.
It's a bit weird to bind data.parent to two different variables here, but I understand why you did it: to make the useSelect below as similar as possible to the original. I think it works nicely for this use case, though I wouldn't mind consolidating into a single one either — whatever you feel is right. Though, if you want to keep it like this, it'd be good to add a comment to explain the why so other readers don't have to discover it themselves.
There was a problem hiding this comment.
though I wouldn't mind consolidating into a single one either
I agree with you! Improved with af5dec2
oandregal
approved these changes
Oct 29, 2024
karthick-murugan
pushed a commit
to karthick-murugan/gutenberg
that referenced
this pull request
Nov 13, 2024
* Data Views: Add Parent field * add comment * improve documentation * fix preview * use number type * enable sorting * move logic in the useMemo function * consolidate data.parent in one variable Co-authored-by: gigitux <[email protected]> Co-authored-by: oandregal <[email protected]>
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.
What?
This PR adds the
Parentfield.Related issue #64519.
Testing Instructions
Ensure that
Quick Edit in DataViewsandQuick Edit in DataViewsare enabled.NoneTesting Instructions for Keyboard
Screenshots or screencast
Screen.Capture.on.2024-10-28.at.17-24-05.mp4