-
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
Make Permalinks editable #3418
Make Permalinks editable #3418
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3418 +/- ##
==========================================
- Coverage 39.08% 31.76% -7.32%
==========================================
Files 290 290
Lines 6967 7706 +739
Branches 1273 1386 +113
==========================================
- Hits 2723 2448 -275
- Misses 3570 4425 +855
- Partials 674 833 +159
Continue to review full report at Codecov.
|
There are a few accessibility concerns with this UI, starting from the placement in the source: it's difficult to tab to it, the only way is to tab backwards. When tabbing forwards, the whole component is rendered when the tab sequence has already "skipped" it so assistive technologies users won't have a clue something appeared on the screen. The button to edit should be a separate control: currently the button is announced based on the slug value, this doesn't give a proper feedback about the button action. The input field is unlabeled. There's no "cancel". In the current interface, the permalink is a fully functional link and it's clickable. Instead, now it's just a span with text styled as a link. However, the first thing to check is probably the input field is actually not editable:
About the UI and interaction, I'd recommend to try to replicate the current one in the Classic editor. There are good reason why each action has a dedicated control. For reference: |
P.S. the demo page crashes when clicking on some blocks, I guess because there's no slug there? |
Yeah, I quickly hacked this into the existing UI to see how it feels - I'm not particularly enamoured by it. It feels super hidden, you can really only discover it by accident. @karmatosed, what are your thoughts on a slug editing UI? |
I have a suggestion (a little bit out there maybe), but I would like to get feedback from @jasmussen on this. If we can, I would like us to use the format we have right now. Here is my suggestion for that without clicking in to make active: The second is when you click in to edit: |
editor/post-permalink/index.js
Outdated
viewLink = link; | ||
if ( samplePermalink ) { | ||
permalink = samplePermalink[ 0 ].replace( '%postname%', samplePermalink[ 1 ] ); | ||
viewLink += '&preview=true'; |
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.
Can we know for certain that the viewLink
already includes other query parameters? Maybe we ought to use addQueryArgs
from the @wordpress/url
package?
https://github.com/WordPress/packages/blob/master/packages/url/README.md
editor/post-permalink/index.js
Outdated
{ ! editingSlug && | ||
<Button | ||
className="editor-post-permalink__slug" | ||
onClick={ this.onEditPermalink } |
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.
We shouldn't offer the option to edit permalink if %postname%
doesn't exist in the permalink structure.
editor/post-permalink/index.js
Outdated
viewLink += '&preview=true'; | ||
} | ||
|
||
const prefix = permalink.replace( /[^/]+\/?$/, '' ), |
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.
Can you explain these regular expressions, preferably in a code comment?
editor/post-permalink/index.js
Outdated
@@ -42,21 +46,68 @@ class PostPermalink extends Component { | |||
}, 4000 ); | |||
} | |||
|
|||
onEditPermalink( event ) { | |||
event.preventDefault(); |
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.
Why do we need to prevent default here?
editor/post-permalink/index.js
Outdated
} | ||
|
||
onCancelEditPermalink( event ) { | ||
event.preventDefault(); |
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.
Why do we need to prevent default here?
@karmatosed I like your screenshot, I think it's an enhancement. However I do agree with both you, Pento and Andrea that this UI (showing the permalink when clicking the title field) is a failed experiment. I can say that because it was my idea ¯\(ツ)/¯ There are some alternative designs in the ticket that I think we should explore (including the link icon). So yes, we absolutely need to make a better UI for it. However the question is whether we need to do it as part of this PR. Perhaps we can do it in a two-parter — first part being to get this PR in, so it's at least editable, and then we make sure to reopen #1285 (or open a new separate ticket) to improve the UI. |
Just to note that in the classic editor, the visible permalink is a fully functional link and it's clickable. Personally, I use it a lot of times to view the post in a new tab, because in the current UI is the most prominent "preview" link for me. |
Thanks for inputs. I am going to take this on and look at what a next version would be design wise. I'll update here once have that. |
I did a little digging into this and went back to the idea of what we have currently: Right now copying is by selecting and copying. We also have a click to view. I thought about what we have in Gutenberg: This is nice because of the simplification. However it loses the ability to edit easily and copying is a step. What about combining? The flow to this would be:
The benefit of this is we keep the minimal UI but we get back some of the features expected. @jasmussen what do you think of this as a suggestion? |
I dig it, and any improvements here are welcome. I don't know that the original issue is solved, though, around discoverability. Perhaps we should revisit the design that had a floating icon symbol next to the title. |
@jasmussen true, it does solve the editable issue though. I do think this icon has a potential: |
I think the stock dashicons link icon could work, yes. Should it be vertically centered in the title box? |
@pento are you up for iterating on this? |
27f4c4d
to
9efecec
Compare
0d9179c
to
88e1e98
Compare
Apart from accessibility concerns to address, there are still some functional and design issues.
Re: the accessibility issues, I'd like to remind UI controls should be grouped in a logical order so I'd recommend to not make the left icon a "Copy" button (it would also miss the confirmation message that appears in place of the original button text). I'd rather place two buttons on the right. Other changes will be necessary to address a11y but I'd say to make it functional first and then improve the a11y part. |
In the Classic editor, when the permalink setting is set to "Plain", the UI is not editable and shows just a button (actually a link) to the Permalink Settings page: Edit: When set to "Numeric", it's not editable and no link is shown: When set to "Custom", it's editable only if it contains the postname, otherwise it's not editable and no link is shown: |
Current "logic": https://core.trac.wordpress.org/browser/trunk/src/wp-admin/includes/post.php?rev=42343#L1367 Looking a that, there are at least a couple more things to address:
|
I pushed some changes to the PR to make the editing of the permalink work all the way through persistence. Right now, the validation is solely done on the server end. The slug is pushed through the REST API, and the server then sanitizes the slug. It returns a modified post object, that contains the (potentially adapted) slug. The Permalink component waits for that response and then adapts its state to accept whatever the server provided. So, as an example, if you already have a post called An obvious improvement would be to add basic sanitization at the client level, but proper sanitization would require roundtrips to the server. I think that this implementation is good enough for a first basic implementation, and further UI/UX iterations should be done before delving too deep into the sanitization rabbit hole. |
The removal seems to have been unintended.
Note: the `<Button>` component does not add the `button` class by default, so I added it manually. See #4638
10664fd
to
8943fff
Compare
It's still a bit buggy, but this PR has been going on way too long. I'm going to merge it, then we can iterate on it a bit more in subsequent PRs. |
There are a number of issues with this that are blockers to releasing. Firstly, if you make an edit, then save, it doesn't take until the second time you edit. Aside from that there are a number of visual issues that diverge form the designs:
|
Reverted with #4675 to proceed with 2.1 release. |
Note: This is a first pass at getting this feature working, it will be iterated on in subsequent PRs.
I guess the branch should be restored? |
I can continue work on this based on the recent feedback. Just a couple of questions:
|
I can restore the branch, though it's my understanding that it's not possible to reopen a merged pull request within GitHub. I'm unsure whether a prompt will become available to create a new pull request from the same branch.
If the components are within the same top-level folder, all components can be accessed via relative paths ( |
Continued in #5414 . |
Description
It's time to make permalinks editable.
Fixes #1285.
Screenshots (jpeg or gifs if applicable):