-
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
Site editor: Add edit page slug field #55767
Conversation
77bcebc
to
d8caf79
Compare
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.
Thanks, Nik! I left a few notes and will do more thorough testing tomorrow.
packages/edit-site/src/components/sidebar-edit-mode/page-panels/page-slug.js
Outdated
Show resolved
Hide resolved
packages/edit-site/src/components/sidebar-edit-mode/page-panels/page-slug.js
Outdated
Show resolved
Hide resolved
Size Change: +2.08 kB (0%) Total Size: 1.7 MB
ℹ️ View Unchanged
|
Nik, I just noticed that we'd need some special handling for draft pages. See: #55774. |
I think everything is handled now. I wasn't expecting to have to deal with so many nuances for this PR 😄 |
packages/edit-site/src/components/sidebar-edit-mode/page-panels/page-slug.js
Outdated
Show resolved
Hide resolved
Yeah. Permalinks are complicated 😅 At least we have a working blueprint from the editor package to use. I've got a few nitpicks/suggestions. Do you mind if I push those changes directly later today? |
Sounds good. The problem here was that we have too much code in |
packages/edit-site/src/components/sidebar-edit-mode/page-panels/page-slug.js
Outdated
Show resolved
Hide resolved
packages/edit-site/src/components/sidebar-edit-mode/page-panels/page-slug.js
Show resolved
Hide resolved
@ntsekouras, it would be nice to get another reviewer on this PR, just in case we missed something; then, I think we're good to merge. |
I removed the "Learn more" and the view link for now and hid the permalink label.
TextControl doesn't yet support this. We could do it when the support is added. |
f432c1b
to
78e28e2
Compare
|
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.
A switch to InputControl
looks like it could work here with a couple of extra props given to it.
TextControl | InputControl |
---|---|
Quick test diff
diff --git a/packages/edit-site/src/components/sidebar-edit-mode/page-panels/page-slug.js b/packages/edit-site/src/components/sidebar-edit-mode/page-panels/page-slug.js
index 55fad12602..5db784991d 100644
--- a/packages/edit-site/src/components/sidebar-edit-mode/page-panels/page-slug.js
+++ b/packages/edit-site/src/components/sidebar-edit-mode/page-panels/page-slug.js
@@ -11,7 +11,8 @@ import { useState, useMemo } from '@wordpress/element';
import { __experimentalInspectorPopoverHeader as InspectorPopoverHeader } from '@wordpress/block-editor';
import { __ } from '@wordpress/i18n';
import {
- TextControl,
+ __experimentalInputControl as InputControl,
+ __experimentalInputControlPrefixWrapper as InputControlPrefixWrapper,
__experimentalHStack as HStack,
__experimentalVStack as VStack,
__experimentalText as Text,
@@ -108,8 +109,14 @@ export default function PageSlug( { postType, postId } ) {
onClose={ onClose }
/>
<VStack spacing={ 5 }>
- <TextControl
+ <InputControl
__nextHasNoMarginBottom
+ prefix={
+ <InputControlPrefixWrapper>
+ /
+ </InputControlPrefixWrapper>
+ }
+ size="__unstable-large"
label={ __( 'Permalink' ) }
hideLabelFromVision
value={ forceEmptyField ? '' : recordSlug }
Thanks! I updated to use InputControl. |
Flaky tests detected in f2089ab. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6768365023
|
Maybe the css didn't load for you - it's here. I removed the prefix, so this should be good to land I guess. |
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.
Thanks for the iterations here @ntsekouras 👍
This PR tests as advertised for me and the styling feedback appears resolved as well.
One further tweak could be to allow pressing enter to save and close the popover. It felt odd while testing that I couldn't do that and it tripped me up a few times. What do you think?
Sounds good. I updated to handle that. |
What?
Part of: #52632
This PR adds an edit page slug field in site editor, in page summary.
Testing Instructions
Screenshots or screencast