Quick Edit - Slug Field: improve slug preview #66559

gigitux · 2024-10-29T13:22:06Z

What?

I noticed that the fallback logic for undefined slugs was missing, for instance draft pages. This PR implements a mechanism to provide a fallback slug when the slug is not defined.

Before	After

Why?

How?

Testing Instructions

Ensure that Quick Edit in DataViews and Quick Edit in DataViews are enabled.
Ensure that you have a draft page without any title set.

Visit the Site Editor.
Click on pages.
Toggle to the table view.
Select the draft page that you created.
Click on the settings.
Click on details panel icon.
Ensure that the link field is visible
Click on it.
Ensure that the slug preview is right. You should see the page ID.
Update the slug.
Save.
Ensure that the page is visible to the new link.

Testing Instructions for Keyboard

Screenshots or screencast

github-actions · 2024-10-29T13:22:20Z

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 props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: gigitux <[email protected]>
Co-authored-by: oandregal <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

oandregal · 2024-10-29T16:56:11Z

How do I trigger the situation this PR fixes? I tried the steps in the description but couldn't. Tried with draft pages (even a new one I just created) but cannot run into the issue. I see the slug everywhere:

Screen.Recording.2024-10-29.at.17.54.57.mov

oandregal · 2024-10-29T16:57:50Z

packages/fields/src/fields/slug/slug-view.tsx


 const SlugView = ( { item }: { item: BasePost } ) => {
-	const slug = item.slug;
+	const slug = item ? getSlug( item ) : '';


getSlug will always return something, even if it's the item.id? Do we need to cover against not returning anything (empty string)? Or perhaps I'm just unable to run into the issue you found?

I didn't have time to investigate why, but during the loading of the page, item is a boolean. You can add print data here:

gutenberg/packages/dataviews/src/dataforms-layouts/panel/index.tsx

Lines 59 to 68 in d871796

function FormField< Item >( {

data,

field,

onChange,

}: FormFieldProps< Item > ) {

// Use internal state instead of a ref to make sure that the component

// re-renders when the popover's anchor updates.

const [ popoverAnchor, setPopoverAnchor ] = useState< HTMLElement | null >(

null

);

Screen.Capture.on.2024-10-30.at.10-44-03.mp4

I see. Can you instead check if item is an object? It just happens to be a boolean, but that check would pass if it's a string only to break later in getSlug. Or integrate that check within getSlug, whatever you think it's best.

A follow-up is that we need a better management of loading states for DataForm, like we have for DataViews. The issue is that the data is not yet loaded. For DataViews we have isLoading prop, it sounds like we need the same for DataForm. Is this something you'd be able to follow-up on?

Actually, wouldn't this logic be best placed in getValue instead of the getSlug utility? That way edit & view components would have access to it.

Improved with 51e08fe.

Is this something you'd be able to follow-up on?

Sure! I created an issue #66599.

It is a good idea, but currently we are not passing to the render component, the field prop. I would create a dedicated issue and craft a dedicated PR for this. If you think that it is not necessary, I'm happy to implement it in this PR too.

Oh, you're right. I thought we did at some point? 🤔 Let's ship and move on.

I just looked this up and it's confusing why the Edit function receives data (Item), field, onChange, hideLabelFromVision but the View function only receives the item (Item). There's also different names (data vs item) for the same prop across functions. I think we need to consolidate and:

The data prop should be called item in both places.

The field should be passed down as a prop in both places.

I created a dedicated issue: #66616

related #67577

gigitux · 2024-10-30T09:47:08Z

How do I trigger the situation this PR fixes? I tried the steps in the description but couldn't. Tried with draft pages (even a new one I just created) but cannot run into the issue. I see the slug everywhere:

Screen.Recording.2024-10-29.at.17.54.57.mov

Sorry for the oversight in the testing instructions. They are now updated. The issue occurs when you have a draft page without a title.

oandregal · 2024-10-30T10:54:36Z

I was able to reproduce this by adding a new Page from the wp-admin flow. If I create one from the site editor, it appears to be created with "no title" as title.

oandregal · 2024-10-30T12:01:29Z

packages/fields/src/fields/slug/slug-view.tsx

@@ -20,7 +21,7 @@ const SlugView = ( { item }: { item: BasePost } ) => {

 	const slugToDisplay = slug || originalSlugRef.current;

-	return `/${ slugToDisplay ?? '' }`;
+	return `${ slugToDisplay ?? '' }`;


We probably no longer need the empty string if we already manage that above?

github-actions · 2024-10-30T14:11:42Z

Flaky tests detected in 51e08fe.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/11594107744
📝 Reported issues:

[Flaky Test] should be able to create a new tag with ' on the name #38828 in /test/e2e/specs/editor/various/taxonomies.spec.js

* Quick Edit - Slug Field: improve slug preview * improve type Co-authored-by: gigitux <[email protected]> Co-authored-by: oandregal <[email protected]>

Quick Edit - Slug Field: improve slug preview

d871796

gigitux self-assigned this Oct 29, 2024

gigitux added [Type] Experimental Experimental feature or API. [Feature] DataViews Work surrounding upgrading and evolving views in the site editor and beyond [Package] DataViews /packages/dataviews labels Oct 29, 2024

gigitux requested review from youknowriad, oandregal and louwie17 October 29, 2024 13:24

oandregal reviewed Oct 29, 2024

View reviewed changes

gigitux requested a review from oandregal October 30, 2024 09:47

oandregal reviewed Oct 30, 2024

View reviewed changes

gigitux mentioned this pull request Oct 30, 2024

DataForm: Improve loading state management #66599

Closed

improve type

51e08fe

gigitux requested a review from oandregal October 30, 2024 13:23

oandregal approved these changes Oct 30, 2024

View reviewed changes

oandregal mentioned this pull request Oct 30, 2024

QuickEdit: Add password field data to the pages quick edit #66567

Merged

gigitux merged commit 8a11ce4 into trunk Oct 30, 2024
68 of 70 checks passed

gigitux deleted the fix/slug-draft-post branch October 30, 2024 18:18

github-actions bot added this to the Gutenberg 19.7 milestone Oct 30, 2024

gigitux mentioned this pull request Oct 30, 2024

DataForm: Consolidate render function argument #66616

Open

oandregal mentioned this pull request Dec 4, 2024

QuickEdit: prevent site-editor from crashing when slug is not an object #67577

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Quick Edit - Slug Field: improve slug preview #66559

Quick Edit - Slug Field: improve slug preview #66559

gigitux commented Oct 29, 2024 •

edited

Loading

github-actions bot commented Oct 29, 2024 •

edited

Loading

oandregal commented Oct 29, 2024

oandregal Oct 29, 2024 •

edited

Loading

gigitux Oct 30, 2024

oandregal Oct 30, 2024 •

edited

Loading

oandregal Oct 30, 2024

gigitux Oct 30, 2024

gigitux Oct 30, 2024

oandregal Oct 30, 2024

oandregal Oct 30, 2024

gigitux Oct 30, 2024

oandregal Dec 4, 2024

gigitux commented Oct 30, 2024

oandregal commented Oct 30, 2024

oandregal Oct 30, 2024

github-actions bot commented Oct 30, 2024

	function FormField< Item >( {
	data,
	field,
	onChange,
	}: FormFieldProps< Item > ) {
	// Use internal state instead of a ref to make sure that the component
	// re-renders when the popover's anchor updates.
	const [ popoverAnchor, setPopoverAnchor ] = useState< HTMLElement \| null >(
	null
	);

Quick Edit - Slug Field: improve slug preview #66559

Quick Edit - Slug Field: improve slug preview #66559

Conversation

gigitux commented Oct 29, 2024 • edited Loading

What?

Why?

How?

Testing Instructions

Testing Instructions for Keyboard

Screenshots or screencast

github-actions bot commented Oct 29, 2024 • edited Loading

oandregal commented Oct 29, 2024

oandregal Oct 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oandregal Oct 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gigitux commented Oct 30, 2024

oandregal commented Oct 30, 2024

Choose a reason for hiding this comment

github-actions bot commented Oct 30, 2024

gigitux commented Oct 29, 2024 •

edited

Loading

github-actions bot commented Oct 29, 2024 •

edited

Loading

oandregal Oct 29, 2024 •

edited

Loading

oandregal Oct 30, 2024 •

edited

Loading