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

Add Editable Permalinks #5756

Merged
merged 63 commits into from Apr 15, 2018

Conversation

@pento
Member

pento commented Mar 23, 2018

Description

#5414 has become way too unwieldy to work with, so this PR is a fresh attempt, based on @karmatosed's design.

Screenshots (jpeg or gifs if applicable):

Initial view of the permalink UI

Editable view of the permalink UI

Edited permalink

Edited permalink after clicking Okay

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code has proper inline documentation.

@pento pento self-assigned this Mar 23, 2018

@pento pento requested a review from WordPress/gutenberg-core Mar 23, 2018

@pento pento referenced this pull request Mar 23, 2018

Closed

Add editable Permalinks #5414

@afercia

This comment has been minimized.

Contributor

afercia commented Mar 23, 2018

Related: #5494

Aside: I see a notice
Notice: Undefined property: WP_Post::$filter in /srv/www/wordpress-develop/public_html/src/wp-includes/class-wp-post.php on line 342

@pento

This comment has been minimized.

Member

pento commented Mar 23, 2018

@afercia: Bug squished, thanks. 🙂

@karmatosed

This comment has been minimized.

Member

karmatosed commented Mar 23, 2018

On this branch I am noticing one thing not like the design, which actually was in the thread and that's copying when clicking the link icon. Here is the conversation for reference:#3418 (comment).

We need to of course test this with notices again but overall seems like the design and on track there.

@pento

This comment has been minimized.

Member

pento commented Mar 23, 2018

Oh, I totally missed that in the conversation. Thanks, @karmatosed!

I turned the Dashicon into a ClipboardButton, so now clicking it copies the permalink to your clipboard. Do you think it needs some sort of feedback after clicking, to show that it copied?

@karmatosed

This comment has been minimized.

Member

karmatosed commented Mar 23, 2018

Do you think it needs some sort of feedback after clicking, to show that it copied?

Good catch, yes can the icon grey out maybe for now until a revisit of that page or refresh? We can consider later if we need a text feedback also.

@pento

This comment has been minimized.

Member

pento commented Mar 23, 2018

Done!

@afercia

This comment has been minimized.

Contributor

afercia commented Mar 23, 2018

Sorry I have to reference also to the a11y feedback given in the previous PR see #3418 (comment)

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.

Also:

  • ideally, icon-only buttons should be avoided
  • if there's really (really, really) the need to use an icon-only button, then it needs a tooltip: please don't use a title attribute
  • the input field must be labeled, it needs at least an aria-label attribute
  • concerns about placement and tab order still stand, and there's #5494 for them

Visually, I'd leave this to @karmatosed but I'm not fully sure about the copy button discoverability: it doesn't look like a button, not sure why users should understand it's an UI control.

Note: the permalink is empty now.

screen shot 2018-03-23 at 14 29 34

@afercia

This comment has been minimized.

Contributor

afercia commented Mar 23, 2018

Link that opens in a new tab/window (target="_blank") should inform users about what's going to happen. There's the ExternalLink component for this, however it also adds an icon.

clearTimeout( this.dismissCopyConfirmation );
componentWillMount() {
if ( this.props.samplePermalinkData ) {
this.updateSamplePermalink( this.props.samplePermalinkData[ 0 ], this.props.samplePermalinkData[ 1 ] );

This comment has been minimized.

@aduth

aduth Mar 23, 2018

Member

This is a once-documented premature optimization anti-pattern, as it's duplicating the source of truth, not necessarily state of the component:

https://web.archive.org/web/20160531222015/http://facebook.github.io:80/react/tips/props-in-getInitialState-as-anti-pattern.html

It will save many a headache to simply calculate the value as used.

This comment has been minimized.

@aduth

aduth Mar 23, 2018

Member

Further, a better optimization we might consider is to create a separate component which manages the editing behaviors, and is only rendered when this.state.editingPermalink is true.

(i.e. why do we calculate this state when I'm merely editing the title's text, or viewing the permalink?)

This comment has been minimized.

@pento

pento Mar 25, 2018

Member

Good point, changed to just calculating when rendering the form.

* @param {string} postName The post slug to use in replacements.
*/
updateSamplePermalink( template, postName ) {
const regex = /%(?:postname|pagename)%/;

This comment has been minimized.

@aduth

aduth Mar 23, 2018

Member

If this is constant, seems we could move it to top-of-file (shared scope) and document appropriately for what it's matching.

this.setState( {
showCopyConfirmation: true,
samplePermalink: template.replace( regex, postName ),
samplePermalinkPrefix: template.split( regex )[ 0 ],

This comment has been minimized.

@aduth

aduth Mar 23, 2018

Member

Aside from potential waste in needlessly calling String#split twice generating the same result, this is a good use case for ES6 array destructuring:

const [ samplePermalinkPrefix, samplePermalinkSuffix ] = template.split( regex );

This comment has been minimized.

@pento

pento Mar 25, 2018

Member

👍🏻

samplePermalink: template.replace( regex, postName ),
samplePermalinkPrefix: template.split( regex )[ 0 ],
samplePermalinkSuffix: template.split( regex )[ 1 ],
postName: postName,

This comment has been minimized.

@aduth

aduth Mar 23, 2018

Member

Minor: This can be shortened to just postName, when property name and variable value match.

See: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Object_initializer#New_notations_in_ECMAScript_2015

This comment has been minimized.

@pento

pento Mar 25, 2018

Member

👍🏻

type="text"
className="editor-post-permalink__edit"
defaultValue={ postName }
onChange={ ( e ) => this.setState( { editedPostName: e.target.value } ) }

This comment has been minimized.

@aduth

aduth Mar 23, 2018

Member

What is e ? error ?

I'm being facetious 😄 but I generally tend to discourage abbreviations where ambiguity is possible.

This comment has been minimized.

@pento

pento Mar 25, 2018

Member

👍🏻

previewLink: getEditedPostPreviewLink( state ),
samplePermalinkData: getEditedPostAttribute( state, 'sample_permalink' ),
permalinkStructure: window.wpApiSettings.schema.permalink_structure,

This comment has been minimized.

@aduth

aduth Mar 23, 2018

Member

A few issues:

  • As much as possible, we shouldn't access the window global directly, since it proliferates the expected global prerequisites beyond that which we're otherwise establishing through the single entry point for initializing an editor
  • This value is not being sourced from the editor state, and as such does not belong in connect's mapStateToProps (not mapped from state)
  • To the previous point, perhaps this should be in editor state, or otherwise provided through context in EditorProvider, where other settings are exposed and later accessed by withContext
  • To this previous point, technically there is context already available for this provided through withAPIData, but since we're not really using API data here, it doesn't seem necessarily appropriate to create a dependency here on this context.

This comment has been minimized.

@pento

pento Mar 25, 2018

Member

Turns out I didn't need that at all, everything was in sample_permalink on the post.

};
},
{
editPost,

This comment has been minimized.

@aduth

aduth Mar 23, 2018

Member

This is fine enough since we already have connect here anyways, but new usage is encouraged (and hopefully simplified) to use withDispatch instead:

import { withDispatch } from '@wordpress/data';
import { Component, compose } from '@wordpress/element';

// ...

export default compose( [
	connect( ( state ) => {
		return {
			isNew: isEditedPostNew( state ),
			previewLink: getEditedPostPreviewLink( state ),
			samplePermalinkData: getEditedPostAttribute( state, 'sample_permalink' ),

			permalinkStructure: window.wpApiSettings.schema.permalink_structure,
		};
	} ),
	withDispatch( ( dispatch ) => {
		const { editPost } = dispatch( 'core/editor' );
		return { editPost };
	} ),
] )( PostPermalink );

This comment has been minimized.

@pento

pento Mar 25, 2018

Member

👍🏻

@@ -34,3 +38,20 @@
@include long-content-fade( $size: 20% );
}
}
.editor-post-permalink__edit-form {
width: 100%;

This comment has been minimized.

@aduth

aduth Mar 23, 2018

Member

This element is technically the child of a flex container and could be sized with flex-basis: 100%; flex-shrink: 1;

This comment has been minimized.

@pento

pento Mar 25, 2018

Member

💪🏻

}
.editor-post-permalink__save {
float: right;

This comment has been minimized.

@aduth

aduth Mar 23, 2018

Member

Maybe some flex instead of float, to avoid the inevitable future headaches:

image

This comment has been minimized.

@pento

pento Mar 25, 2018

Member

💪🏻

permalinkStructure && ! editingPermalink &&
<Button
className="editor-post-permalink__edit button"
onClick={ () => this.setState( { editingPermalink: true } ) }

This comment has been minimized.

@aduth

aduth Mar 23, 2018

Member

Because we only set editedPostName when changing input, if one presses "Edit" and then immediately "Ok", the permalink is reset to empty.

This comment has been minimized.

@pento

pento Mar 25, 2018

Member

👍🏻

@aduth

This comment has been minimized.

Member

aduth commented Mar 23, 2018

Mobile isn't looking so great:

Mobile

@aduth

This comment has been minimized.

Member

aduth commented Mar 23, 2018

Link that opens in a new tab/window (target="_blank") should inform users about what's going to happen.

Perhaps we can build this behavior into the base Button component so it's not a consideration for the developer.

@pento

This comment has been minimized.

Member

pento commented Mar 25, 2018

Perhaps we can build this behavior into the base Button component so it's not a consideration for the developer.

Yah, I'm inlined to go this way. It's not an external link, so it seems weird to be using the ExternalLink component.

Also, thanks to the magic of flexbox, mobile is significantly less unusably ugly now.

Mobile view of the permalink editor

@pento

This comment has been minimized.

Member

pento commented Mar 25, 2018

Is there a selector like getEditedPostAttribute(), except getEditedPostAttributeAfterLast REQUEST_POST_UPDATE_SUCCESSAfterWhichFallBackToCurrentPostAttribute()?

@afercia

This comment has been minimized.

Contributor

afercia commented Mar 26, 2018

Quickly tested: while it seems to work with new posts:

screen shot 2018-03-26 at 09 08 15

it doesn't seem to work well with existing posts:

screen shot 2018-03-26 at 09 06 54

  • the permalink is false
  • my permalinks setting is Month and name but the UI is showing me the link "Change permalinks" that should appear only when permalinks are set to "Plain"

From an accessibility perspective, I see part of the feedback is ignored. Navigating content for keyboard users and assistive technologies users is often a linearized experience. Content should be placed in a logical order, taking into account linearization. In this regard, the previous PR was better because it had in order:

  • the "what" (the permalink), i.e. the labeled editable field
  • the available actions: the two buttons "copy" and "edit"

screen shot 2018-03-26 at 09 19 21

(of course, "Copy" shouldn't be a primary button and some details were still unpolished, but the UI in the previous PR was better IMHO)

@pento

This comment has been minimized.

Member

pento commented Mar 26, 2018

the permalink is false

Ooops. 🙃

From an accessibility perspective, I see part of the feedback is ignored.

Not ignored, you mentioned you were going to look at it in #5494, so I assumed the action was going to happen there. I'll see if I can make some changes in this PR, too.

@pento

This comment has been minimized.

Member

pento commented Mar 26, 2018

@afercia: I've just pushed a few tweaks to the keyboard order, can you give some feedback on that?

I used autofocus on the <input>, which I know isn't ideal for accessibility, but it seems like this is an appropriate usage?

@afercia

This comment has been minimized.

Contributor

afercia commented Mar 26, 2018

Thanks @pento. Some a11y considerations:

As long as it works in supported browsers, I think using autofocus is fine in this case, as this mini-UI has the specific task to edit the permalink

The buttons placement is still not ideal, and it's a violation of a specific WCAG guideline: the visual order should always match the DOM order:
Success Criterion 1.3.2 Meaningful Sequence
https://www.w3.org/TR/WCAG21/#meaningful-sequence
https://www.w3.org/WAI/WCAG21/Understanding/meaningful-sequence.html
https://www.w3.org/TR/WCAG20-TECHS/C27

Tooltips work in an accessible way out of the box when used for the IconButton component with the label prop, as that produces both an aria-label and the tooltip. Since this is a particular button, I guess there's the need to explicitly pass an aria-label with the same string to the button. Currently, the button is not labeled: no text or aria-label. /cc @aduth maybe there's a way to make other buttons behave like the IconButton?

Switching components on the fly produces a focus loss:

  • when editing, when focus is on the Edit OK button
  • press Enter
  • the whole "editor" component is replaced so the previous focused element is removed from the DOM and focus is lost. This is particularly evident in IE11 but happens also in modern browsers: they're just a bit smarter and try to keep focus in place but actually there's no focused element at some point. It would be great to try to avoid this and just update the props of the button, if possible.

I'd remove the dot at the end of the string :)

Note: I've edited the focus loss issue description, I had described a wrong scenario.

@aduth

This comment has been minimized.

Member

aduth commented Mar 26, 2018

Tooltips work in an accessible way out of the box when used for the IconButton component with the label prop, as that produces both an aria-label and the tooltip. Since this is a particular button, I guess there's the need to explicitly pass an aria-label with the same string to the button. Currently, the button is not labeled: no text or aria-label. /cc @aduth maybe there's a way to make other buttons behave like the IconButton?

If it's reasonable to apply broadly, I think we should pull this behavior out of IconButton specifically and make it built-in to the tooltip component. In other words, when wrapping an element with Tooltip, we add any necessary attributes to the element to reflect information that the Tooltip provides (aria-label? aria-describedby?)

@pento

This comment has been minimized.

Member

pento commented Mar 27, 2018

Thanks for the extra info, @afercia.

As it's just a shiny new kind of accessibility issue, I've reverted the DOM order of the copy button back to it's original place: @karmatosed previously mentioned that the design in this PR is a starting point, so let's look at how we can order these things in the next iteration.

When the edit form is closed, it now focusses on the permalink, which seems to be the most relevant element to shift the focus to.

From an accessibility perspective, how do you feel about the current state of this PR as a place to iterate from? There are certainly improvements to be made, but it seems like they are better made as part of the design iterations, as they depend on how things are laid out.

@aduth: could you review the code again?

@karmatosed: I think I've caught everything, but can you confirm that the current design of this PR matches what you had in mind?

@afercia

This comment has been minimized.

Contributor

afercia commented Mar 27, 2018

@pento thanks for the changes. The most important thing is to avoid the focus loss and that is addressed now (we can iterate on the better place to move focus to). Not sure @aduth
will like querySelector :D as far as I can tell refs should be used for this purpose.

As per the placement of elements, yep this is a case where design and accessibility have different needs. As you suggested I think we can iterate in #5494 even if that issue was meant to improve the placement and tab order of the whole UI, I think it can focus also on the UI content.

@karmatosed I'd suggest to consider the link icon doesn't really look like a button, I have concerns on its discoverability. Can we consider a standard button, for better clarity for all users?

@afercia

This comment has been minimized.

Contributor

afercia commented Mar 27, 2018

Something to consider:

When the URL is very long, it fades out on the right:

screen shot 2018-03-27 at 09 23 04

Instead, on the current UI, the "permalink" is truncated in the middle, allowing users to see at least the last part. Not ideal either, but probably better:

screen shot 2018-03-27 at 10 15 54

@GaryJones

This comment has been minimized.

Contributor

GaryJones commented Mar 27, 2018

When the input field is shown, there's a trailing / shown after it.

Is this making an assumption that a pretty permalink always contains a trailing /, or does it actually check the permalink format?

pento added some commits Apr 9, 2018

@pento pento added this to the 2.7 milestone Apr 15, 2018

@pento

This comment has been minimized.

Member

pento commented Apr 15, 2018

This has been fun, y'all. Let's do polish things in follow up PRs.

@pento pento merged commit 4c90150 into master Apr 15, 2018

2 checks passed

codecov/project 44.3% (-0.16%) compared to 0c5aa0d
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@pento pento deleted the add/revenge-of-the-editable-permalinks branch Apr 15, 2018

@aduth aduth referenced this pull request Jun 25, 2018

Merged

fix: Show permalink editor in editor #7494

4 of 4 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment