Skip to content
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.

Sign up for GitHub

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

Quick Edit - Slug Field: improve slug preview #66559

Merged
merged 2 commits into from
Oct 30, 2024
Merged

Quick Edit - Slug Field: improve slug preview #66559

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d871796
Quick Edit - Slug Field: improve slug preview
gigitux Oct 29, 2024
51e08fe
improve type
gigitux Oct 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 21 additions & 24 deletions packages/fields/src/fields/slug/slug-edit.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import { __ } from '@wordpress/i18n';
* Internal dependencies
*/
import type { BasePost } from '../../types';
import { getSlug } from './utils';

const SlugEdit = ( {
field,
Expand All @@ -29,7 +30,7 @@ const SlugEdit = ( {
}: DataFormControlProps< BasePost > ) => {
const { id } = field;

const slug = field.getValue( { item: data } ) ?? '';
const slug = field.getValue( { item: data } ) || getSlug( data );
const permalinkTemplate = data.permalink_template || '';
const PERMALINK_POSTNAME_REGEX = /%(?:postname|pagename)%/;
const [ prefix, suffix ] = permalinkTemplate.split(
Expand Down Expand Up @@ -115,30 +116,26 @@ const SlugEdit = ( {
}
} }
aria-describedby={ postUrlSlugDescriptionId }
help={
<>
<p className="fields-controls__slug-help">
<span className="fields-controls__slug-help-visual-label">
{ __( 'Permalink:' ) }
</span>
<ExternalLink
className="fields-controls__slug-help-link"
href={ permalink }
>
<span className="fields-controls__slug-help-prefix">
{ permalinkPrefix }
</span>
<span className="fields-controls__slug-help-slug">
{ slugToDisplay }
</span>
<span className="fields-controls__slug-help-suffix">
{ permalinkSuffix }
</span>
</ExternalLink>
</p>
</>
}
/>
<div className="fields-controls__slug-help">
<span className="fields-controls__slug-help-visual-label">
{ __( 'Permalink:' ) }
</span>
<ExternalLink
className="fields-controls__slug-help-link"
href={ permalink }
>
<span className="fields-controls__slug-help-prefix">
{ permalinkPrefix }
</span>
<span className="fields-controls__slug-help-slug">
{ slugToDisplay }
</span>
<span className="fields-controls__slug-help-suffix">
{ permalinkSuffix }
</span>
</ExternalLink>
</div>
</VStack>
) }
{ ! isEditable && (
Expand Down
5 changes: 3 additions & 2 deletions packages/fields/src/fields/slug/slug-view.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,10 @@ import { useEffect, useRef } from '@wordpress/element';
* Internal dependencies
*/
import type { BasePost } from '../../types';
import { getSlug } from './utils';

const SlugView = ( { item }: { item: BasePost } ) => {
const slug = item.slug;
const slug = item ? getSlug( item ) : '';
Copy link
Member

@oandregal oandregal Oct 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@oandregal oandregal Oct 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Improved with 51e08fe.

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

Sure! I created an issue #66599.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. The data prop should be called item in both places.
  2. The field should be passed down as a prop in both places.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I created a dedicated issue: #66616

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

related #67577

const originalSlugRef = useRef( slug );

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

const slugToDisplay = slug || originalSlugRef.current;

return `/${ slugToDisplay ?? '' }`;
return `${ slugToDisplay ?? '' }`;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

};

export default SlugView;
15 changes: 15 additions & 0 deletions packages/fields/src/fields/slug/utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
/**
* WordPress dependencies
*/
import { cleanForSlug } from '@wordpress/url';
/**
* Internal dependencies
*/
import type { BasePost } from '../../types';
import { getItemTitle } from '../../actions/utils';

export const getSlug = ( item: BasePost ) => {
return (
item.slug || cleanForSlug( getItemTitle( item ) ) || item.id.toString()
);
};
Loading