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

Add permalinks to the document sidebar #11874

Merged
merged 4 commits into from
Nov 15, 2018
Merged

Conversation

jorgefilipecosta
Copy link
Member

Description

Closes: #11858
This PR adds a permalink panel to the document sidebar, trying to follow the design specified in #11858.

This PR is the first iteration so that we can iterate on the design in parallel.
One missing nice feature is the ability to auto refresh the permalink settings (prefix/suffix) if the settings are changed in another tab, I'm not sure if this feature is a blocker and I wanted to think a little bit better about this part.

How has this been tested?

Verify that if the post is new, the panel does not appear. Some behavior we have on the existing permalink edit UI (close to title).
Verify that if the post does not contains editable permalinks (e.g.: Plain permalink setting) the input is not available and only the post link is shown.
Verify that if the permalink is editable it is possible to edit the permalink without problems and after saving the post the permalink is correctly changed/saved.

Screenshots

screenshot 2018-11-14 at 17 57 30

screenshot 2018-11-14 at 18 19 20

screenshot 2018-11-14 at 18 45 37

@jorgefilipecosta jorgefilipecosta added the Needs Design Feedback Needs general design feedback. label Nov 14, 2018
@jorgefilipecosta jorgefilipecosta added this to the 4.4 milestone Nov 14, 2018
@youknowriad
Copy link
Contributor

In theory we are in a design freeze moment, but from my perspective the current permalink UI is buggy and not discoverable, I consider this PR as a bug fix.

href={ addQueryArgs( 'options-permalink.php' ) }
target="_blank"
>
{ __( 'Permalink Settings' ) }
Copy link
Member

@Soean Soean Nov 14, 2018

Choose a reason for hiding this comment

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

Only show it to user roles who are able to change the settings.

}

.edit-post-post-link__link {
word-break: break-word;
Copy link
Member

Choose a reason for hiding this comment

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

Not supported in Firefox, see https://caniuse.com/#feat=word-break notes

Suggested change
word-break: break-word;
word-wrap: break-word;

@Soean
Copy link
Member

Soean commented Nov 14, 2018

If the post type has the argument 'publicly_queryable' => false, we should hide the post-link panel.

@karmatosed karmatosed removed the Needs Design Feedback Needs general design feedback. label Nov 15, 2018
@youknowriad youknowriad modified the milestones: 4.4, 4.5 Nov 15, 2018
@karmatosed
Copy link
Member

karmatosed commented Nov 15, 2018

I have removed design feedback as this has had a review.

@youknowriad
Copy link
Contributor

I think the idea is to remove the current permalink UI as well.

@mtias mtias added the [Feature] Document Settings Document settings experience label Nov 15, 2018
Copy link
Member

@karmatosed karmatosed left a comment

Choose a reason for hiding this comment

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

I think we need the icon to indicate it's a link that goes out (similar to links) for accessibility, but beyond that approving design. I do wonder why it wasn't an open panel for me, all other document panels were open so it was odd to have this new one not open. I get the logic though.

@jorgefilipecosta
Copy link
Member Author

jorgefilipecosta commented Nov 15, 2018

I think the idea is to remove the current permalink UI as well.

Hi @youknowriad the issue specifies:

(@melchoyce worked on a suggested design for this which adds to the document settings (note this adds doesn't remove from title for now):

That was the reason why I did not remove the UI from the title yet.

@spencerfinnell
Copy link

It makes sense to me to remove it from the content. It's certainly a "Document" setting and is not relevant to blocks (until the Post Title becomes its own block).

@mtias mtias modified the milestones: 4.5, 4.4 Nov 15, 2018
@jorgefilipecosta
Copy link
Member Author

If the post type has the argument 'publicly_queryable' => false, we should hide the post-link panel.

I added an isViewable check in my tests if CPT has publicly_queryable false isViewable also becomes false.

@jorgefilipecosta jorgefilipecosta merged commit b1e11e5 into master Nov 15, 2018
@jorgefilipecosta jorgefilipecosta deleted the add/post-link-panel branch November 15, 2018 16:10
@earnjam
Copy link
Contributor

earnjam commented Nov 15, 2018

I was about to open PR to resolve #7002 and #7129 as well as give it feature parity with the classic editor. Code is basically ready—I was literally writing out the description right now.

Both of those things are still issues with this new document sidebar panel, but @mtias mentioned the old UI would be removed in #10996.

Should I refocus on fixing them in this new panel? Or just abandon the effort?

@melchoyce
Copy link
Contributor

I think we need the icon to indicate it's a link that goes out (similar to links) for accessibility

That makes sense to me 👍

@afercia
Copy link
Contributor

afercia commented Nov 18, 2018

Noticed this PR a bit late (and only because #5494 was closed) so there was no chance to give some accessibility feedback so far. At a first glance, there are a few things that could be improved.

1
When creating a new post, the whole panel is not rendered yet in the sidebar. The panel appears only after the first auto-save or after users click on "Save draft". This could be confusing for users who are just exploring the new post screen and wondering where the permalink setting is. It could be even more confusing for screen reader users, as screen readers have special tools to explore a web page (e.g. list all the headings, list all the form controls, list all the links, etc.) and they won't find anything related to the permalink setting. Same potential confusion applies to low vision users, screen magnifier users, users with cognitive impairments, etc. I'd suggest to consider to always show the panel in the sidebar with some meaningful message when a slug doesn't exist yet. Will open a new issue for this.

2
I'd tend to think the input field label URL is a bit confusing, as users might think they have to fill the field with a complete URL starting with http(s).

screenshot 2018-11-18 at 14 27 02

I've read #11858 (comment) and it makes perfectly sense to avoid technical terms. However, the label could be improved a bit. Also noting Squarespace does use some form of "URL" label but it also expands it adding "slug":

screenshot 2018-11-18 at 13 55 29

I have no strong opinions on which the best wording would be, but I'd suggest to consider a more meaningful one. Will open a new issue for this.

3
The preview and "Permalink Settings" links use target="_blank" but users are not informed the link is going to open in a new browser's tab. Gutenberg has a component for this: ExternalLink, which also adds an icon. I'm going to push a quick PR to address this issue.

4
Lastly, the numeric value to use for a bold font-weight is 600, see #11687. Will address this in the PR for point 3.

@earnjam
Copy link
Contributor

earnjam commented Nov 18, 2018

@afercia I addressed a few of those things (like showing on first load instead of on save) in #12009 for the title permalink editor.

Ideally if we are keeping both then they would share components since the logic is not straightforward. If we are removing the title permalink editor, then we could port the logic from my PR to this component.

@afercia
Copy link
Contributor

afercia commented Nov 18, 2018

@earnjam thanks! It's not so clear to me what is the process to deprecate the previous permalink UI and when it will be removed, I'm afraid I can't help so much in making a decision.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Document Settings Document settings experience
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add permalinks to the document sidebar