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.

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

Merged
merged 22 commits into from
Jan 24, 2018
Merged

Make Permalinks editable #3418

merged 22 commits into from
Jan 24, 2018

Conversation

pento
Copy link
Member

@pento pento commented Nov 10, 2017

Description

It's time to make permalinks editable.

Fixes #1285.

Screenshots (jpeg or gifs if applicable):

viewing permalink

editing permalink

@pento pento added Core REST API Task Task for Core REST API efforts Needs Design Needs design efforts. labels Nov 10, 2017
@codecov
Copy link

codecov bot commented Nov 10, 2017

Codecov Report

Merging #3418 into master will decrease coverage by 7.31%.
The diff coverage is 0%.

Impacted file tree graph

@@            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
Impacted Files Coverage Δ
editor/post-permalink/index.js 0% <0%> (ø)
components/notice/index.js 0% <0%> (-100%) ⬇️
components/panel/row.js 0% <0%> (-100%) ⬇️
blocks/block-controls/index.js 0% <0%> (-100%) ⬇️
blocks/url-input/button.js 0% <0%> (-100%) ⬇️
blocks/alignment-toolbar/index.js 16.66% <0%> (-83.34%) ⬇️
editor/components/post-last-revision/check.js 0% <0%> (-75%) ⬇️
editor/components/post-taxonomies/index.js 28.57% <0%> (-57.15%) ⬇️
editor/components/document-outline/index.js 17.85% <0%> (-57.15%) ⬇️
blocks/block-alignment-toolbar/index.js 33.33% <0%> (-55.56%) ⬇️
... and 298 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5aabd70...9efecec. Read the comment docs.

@afercia
Copy link
Contributor

afercia commented Nov 10, 2017

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:

Warning: Failed form propType: You provided a value prop to a form field without an onChange handler. This will render a read-only field. If the field should be mutable use defaultValue. Otherwise, set either onChange or readOnly.

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:

screen shot 2017-11-10 at 16 06 10

screen shot 2017-11-10 at 16 07 05

screen shot 2017-11-10 at 16 13 05

@afercia
Copy link
Contributor

afercia commented Nov 10, 2017

P.S. the demo page crashes when clicking on some blocks, I guess because there's no slug there?

@pento
Copy link
Member Author

pento commented Nov 10, 2017

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?

@karmatosed karmatosed self-assigned this Nov 13, 2017
@karmatosed
Copy link
Member

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:

1

The second is when you click in to edit:

2

viewLink = link;
if ( samplePermalink ) {
permalink = samplePermalink[ 0 ].replace( '%postname%', samplePermalink[ 1 ] );
viewLink += '&preview=true';
Copy link
Member

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

{ ! editingSlug &&
<Button
className="editor-post-permalink__slug"
onClick={ this.onEditPermalink }
Copy link
Member

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.

viewLink += '&preview=true';
}

const prefix = permalink.replace( /[^/]+\/?$/, '' ),
Copy link
Member

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?

@@ -42,21 +46,68 @@ class PostPermalink extends Component {
}, 4000 );
}

onEditPermalink( event ) {
event.preventDefault();
Copy link
Member

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?

}

onCancelEditPermalink( event ) {
event.preventDefault();
Copy link
Member

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?

@jasmussen
Copy link
Contributor

jasmussen commented Nov 15, 2017

@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.

@afercia
Copy link
Contributor

afercia commented Nov 15, 2017

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.
In the new editor this will change, because the permalink UI will be displayed only when needed, but I'd consider to keep the link clickable. The last part shouldn't be a button or input field by default. It should transform in an input field when editing and there should be a separate "Edit" button.

@karmatosed
Copy link
Member

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.

@karmatosed
Copy link
Member

I did a little digging into this and went back to the idea of what we have currently:

permalinks-existing

Right now copying is by selecting and copying. We also have a click to view. I thought about what we have in Gutenberg:

permalinks-gutenberg

This is nice because of the simplification. However it loses the ability to edit easily and copying is a step. What about combining?

permalinks-suggestion

The flow to this would be:

  • Click to change permalinks: this is a useful feature for users and I think worth putting back in, at least until we can find a better place for it.
  • Editing is a one step with an 'ok' being closing and cancelling. This works as both get the same end result. Saving would happen only if a change had happened. Over having a cancel button.
  • Clicking the permalink shows it just like in the original version.

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?

@jasmussen
Copy link
Contributor

@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.

@karmatosed
Copy link
Member

@jasmussen true, it does solve the editable issue though. I do think this icon has a potential:

permalink1

@jasmussen
Copy link
Contributor

I think the stock dashicons link icon could work, yes. Should it be vertically centered in the title box?

@karmatosed
Copy link
Member

@pento are you up for iterating on this?

@amandablum
Copy link

amandablum commented Dec 18, 2017

how can I help? (other than to test, which I'm happy to do)
Image of Scary Mervyn's Lady

@pento pento force-pushed the add/1285-editable-permalinks branch from 27f4c4d to 9efecec Compare December 22, 2017 05:36
@karmatosed karmatosed added this to the 2.0.0 milestone Jan 3, 2018
@karmatosed karmatosed assigned pento and unassigned karmatosed Jan 3, 2018
@pento pento force-pushed the add/1285-editable-permalinks branch from 0d9179c to 88e1e98 Compare January 8, 2018 06:52
@karmatosed
Copy link
Member

This seems ready to me to get merged with what we have. One potential improvement would be for the link icon to be a copy of the url. I actually found myself wanting to do that. Right now you can't highlight and copy from within Gutenberg the url easily, it clicks to show and you can't copy.

2018-01-10 at 11 04

@afercia
Copy link
Contributor

afercia commented Jan 10, 2018

Apart from accessibility concerns to address, there are still some functional and design issues.

  • the slug field is still not editable (JS warning missing onChange handler)
  • I have no idea how the permalink gets saved 🙂
  • when a notice is displayed, for example "Post updated") the whole UI is hidden behind the notice, only a tiny part of the button is barely visible:

screen shot 2018-01-10 at 19 43 43

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.

@schlessera
Copy link
Member

It looks like the permalink is editable even if permalinks are not being used:
image 2018-01-11 at 4 03 59 pm
This should obviously not be possible, I guess the Edit button should then be greyed out or hidden.

@afercia
Copy link
Contributor

afercia commented Jan 11, 2018

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:

screen shot 2018-01-11 at 16 52 59

Edit:

When set to "Numeric", it's not editable and no link is shown:

screen shot 2018-01-11 at 16 55 24

When set to "Custom", it's editable only if it contains the postname, otherwise it's not editable and no link is shown:

screen shot 2018-01-11 at 16 56 34

@afercia
Copy link
Contributor

afercia commented Jan 11, 2018

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:

  • it should check also for '%pagename%' and not just for '%postname%'
  • when it's a page and it's the front page, the permalink is not editable and doesn't show the link to the permalinks settings

@afercia afercia mentioned this pull request Jan 11, 2018
@schlessera
Copy link
Member

schlessera commented Jan 12, 2018

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 test, and you rename another post to test as well, it will change to test-2 a few moments after saving the post.

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.

@pento pento force-pushed the add/1285-editable-permalinks branch from 10664fd to 8943fff Compare January 24, 2018 11:16
@pento
Copy link
Member Author

pento commented Jan 24, 2018

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.

@pento pento merged commit 5384466 into master Jan 24, 2018
@pento pento deleted the add/1285-editable-permalinks branch January 24, 2018 12:24
@karmatosed
Copy link
Member

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:

  • The input area is out of line.
  • There are 2 buttons : copy and edit beside each other the same styling.

aduth added a commit that referenced this pull request Jan 25, 2018
gziolo pushed a commit that referenced this pull request Jan 25, 2018
@gziolo
Copy link
Member

gziolo commented Jan 25, 2018

Reverted with #4675 to proceed with 2.1 release.

mcsf pushed a commit that referenced this pull request Jan 25, 2018
Note: This is a first pass at getting this feature working, it will be iterated on in subsequent PRs.
@afercia
Copy link
Contributor

afercia commented Jan 26, 2018

I guess the branch should be restored?

@schlessera
Copy link
Member

I can continue work on this based on the recent feedback. Just a couple of questions:

  • Will the branch be restored or shall I create a new PR?
  • Is there a reference somewhere of what components are meant to be used, and when or how? It is difficult to find out what can be reused to base new code on.

@aduth
Copy link
Member

aduth commented Jan 26, 2018

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.

Is there a reference somewhere of what components are meant to be used, and when or how? It is difficult to find out what can be reused to base new code on.

If the components are within the same top-level folder, all components can be accessed via relative paths (import from '../../'). For other top-level folders, it depends on whether it's exported (usually from its index.js), but generally I'd say most components are fair to use. That said, there's a loose hierarchy of decreasing specificity from edit-post (post editor) > editor (general block editor) > blocks (blocks API, library), and these components should ideally not be consuming upwards in the hierarchy (i.e. the generalized block editor editor should not be using components from the post editor edit-post).

@aduth aduth restored the add/1285-editable-permalinks branch January 26, 2018 18:07
@atimmer atimmer mentioned this pull request Feb 1, 2018
3 tasks
@mtias mtias mentioned this pull request Feb 7, 2018
9 tasks
schlessera added a commit that referenced this pull request Mar 5, 2018
…ditable-permalinks-2

The previous pull-request under #3418 cannot be properly opened again to continue work on it, as it had been merged already.

This new PR continues work on solving #1285.

Please refer to #3418 for design mockups and prior history.
@schlessera
Copy link
Member

Continued in #5414 .

@gziolo gziolo deleted the add/1285-editable-permalinks branch March 5, 2018 21:12
@pento pento mentioned this pull request Mar 23, 2018
3 tasks
@designsimply designsimply added the [Feature] Permalink The permalink of a post or page and the experience of setting or editing it label Nov 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Core REST API Task Task for Core REST API efforts [Feature] Permalink The permalink of a post or page and the experience of setting or editing it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants