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 a label to the URL field in the Publishing Flow panel #9536

Merged
merged 2 commits into from
Oct 12, 2018

Conversation

afercia
Copy link
Contributor

@afercia afercia commented Sep 1, 2018

Description

The Post URL field in the Publishing Flow panel is unlabelled. This PR adds a visible label. Also simplifies a bit the text for the Scheduled posts. I know there are pending proposal to improve and add features to the publishing flow so this part will be very likely improved and iterated. The most important thing in this PR is the input field label.

Screenshots

Before:

screen shot 2018-09-01 at 14 16 53

After:

screen shot 2018-09-01 at 15 46 11

Note 1:
the sentence "The post address will be:" is not appropriate anyways, as a post can be of different types: post, page, or a custom post type. To keep things simple I've just removed it in favor of "What’s next?".

Note 2:
since the URL field is readonly, I'd suggest to communicate to users it can't be edited. Usually in WordPress this is done by using a light gray background. Something like in the screenshot below or a lighter gray could work:

screen shot 2018-09-01 at 16 29 05

Fixes #9535

@afercia afercia added the [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). label Sep 1, 2018
@pento pento added the Needs Design Feedback Needs general design feedback. label Sep 3, 2018
<p className="post-publish-panel__postpublish-subheader">
<strong>{ __( 'What’s next?' ) }</strong>
</p>
<label htmlFor={ id } className="post-publish-panel__postpublish-link-label">
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 use Text Control component to simplify this logic? (it automatically generates the ids).

Copy link
Member

@jorgefilipecosta jorgefilipecosta left a comment

Choose a reason for hiding this comment

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

Hi @afercia thank you for looking into this issue. I did some tests, and things worked correctly 👍 From the code point view the changes look good, as a possible improvement we may check if Text Control component can be used here to simplify things and promote reusability.

@afercia
Copy link
Contributor Author

afercia commented Sep 4, 2018

Hi @jorgefilipecosta, thanks. Updated to use TextControl. I've also added a light grey background to the readonly field. I've opted for $light-gray-400 #e8eaeb because it's the closest one to the grey used in core (#eeeeee).

One thing I don't like so much in the TextControl component is that the label element, by default, is styled as display: block. Thus, all the available width is clickable:

textcontrol label

This could be confusing for users, especially when the clickable area is very large. Should go in a separate issue though.

Note:
I see a few warnings (not recognized props on DOM elements) when the Publish panel opens, seem unrelated to me and they happen also on master.

@karmatosed
Copy link
Member

Looks like there are some conflicts on this. I am approving design wise but we would need to get those resolve, thanks!

@karmatosed karmatosed removed the Needs Design Feedback Needs general design feedback. label Oct 11, 2018
@jorgefilipecosta jorgefilipecosta force-pushed the update/publishing-flow-url-field-label branch from d21eb39 to 58e8b3f Compare October 11, 2018 22:23
Copy link
Member

@jorgefilipecosta jorgefilipecosta left a comment

Choose a reason for hiding this comment

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

I rebased this PR (resolving the conflict) so the local tests take into account changes occurred in parallel.
The tests went well and the code looks good to me. Thank you for iterating on this changes 👍

@afercia
Copy link
Contributor Author

afercia commented Oct 12, 2018

Thank you Jorge.

@afercia afercia merged commit 0b30c73 into master Oct 12, 2018
@afercia afercia deleted the update/publishing-flow-url-field-label branch October 12, 2018 16:42
@karmatosed
Copy link
Member

Should we consider URL over address? How does address scale for all situations? I'm not debating having the word as much as having some copy iterations.

@chrisvanpatten
Copy link
Member

@karmatosed I also wonder if it's even better to switch over to "Permalink" since that's a standard and familiar WP term already used in a few places.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants