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

Update spacing in prepublish panel titles #7844

Merged
merged 1 commit into from
Jul 10, 2018

Conversation

sarahmonster
Copy link
Member

@sarahmonster sarahmonster commented Jul 9, 2018

Description

Adds missing spaces to the prepublish panel titles, fixing #7842.

Before:

screenshot 2018-07-09 20 06 39

After:

screenshot 2018-07-09 20 05 07

How has this been tested?

Tested on my local install Mac Chrome/Firefox, using English and Hebrew to test RTL-conversion.

Types of changes

Super-quick three-line bug fix!

Checklist:

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

@tofumatt tofumatt self-requested a review July 10, 2018 08:31
Copy link
Member

@tofumatt tofumatt left a comment

Choose a reason for hiding this comment

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

We should keep the space in the string, but add the padding in. I'll make the change and push a fix to this branch.

@@ -30,13 +30,13 @@ function PostPublishPanelPrepublish( {
{ hasPublishAction && (
<Fragment>
<PanelBody initialOpen={ false } title={ [
__( 'Visibility: ' ),
Copy link
Member

Choose a reason for hiding this comment

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

I think the whitespace should stay in these translation strings because it'll make screen readers, copy pasters, and semantic nitpickers like me happier that it's there. Technically the space is part of the translation here.

(See: #7842 (comment))

@tofumatt
Copy link
Member

Alternatively, you could add markup and make the text with the colon a header. Apparently in the handbook we shouldn't be using trailing spaces in the string, as @sarahmonster pointed out in the comment, so we should wrap those bits of text in an <hx> tag (whatever level makes sense there).

You can't really copy/paste those sections easily so that's not a concern, but technically we shouldn't have Visibility:<span>Public</span> in the markup, which this change would result in.

Copy link
Member

@tofumatt tofumatt left a comment

Choose a reason for hiding this comment

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

Hmm, it looks like either way the markup here isn't really great… and this fix at least:

  1. respects whitespace rules for i18n strings
  2. looks better

So actually let's 🚢 but maybe file a follow-up issue to deal with this markup… it feels like it should be a dl or maybe all the option names (eg "Visibility") should have a tag around them to separate them from the setting value (eg "Public").

@sarahmonster sarahmonster added [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). Internationalization (i18n) Issues or PRs related to internationalization efforts labels Jul 10, 2018
@sarahmonster
Copy link
Member Author

I'm a bit unsure of the best solution here, both from an internationalisation and an accessibility point of view. I could wrap the other part of the title in a span, so the markup looks like this:

<span>Visibility:</span><span>Public</span>

...but I don't know if that really solves the issue at all, and we'd still need to use CSS for the space. I could hard-code a space in there, but that wouldn't work for RTL languages:

<span>Visibility:</span>&nbsp;<span>Public</span>

I suspect the best way of handling it would be to nix the CSS space and instead send the whole string, including the label, to the translation function, but that doesn't work either, because we'd need to pass in a React component:

<PanelBody initialOpen={ false } title={
	sprintf(
	           __( 'Visibility: %s' ),
                  <span className="editor-post-publish-panel__link" key="label"><PostVisibilityLabel /></span>
		)
}>

Perhaps there's a better approach here that I just haven't thought of yet?

@tofumatt
Copy link
Member

tofumatt commented Jul 10, 2018 via email

@tofumatt tofumatt added this to the 3.3 milestone Jul 10, 2018
@tofumatt tofumatt merged commit 6b746e0 into master Jul 10, 2018
@tofumatt tofumatt deleted the update/spacing-in-prepublish-panel-titles branch July 10, 2018 17:32
@tofumatt
Copy link
Member

Tested this in an RTL locale and it looked okay as well; gonna ship it because it's nicer than what we had but we can follow-up with more specific issues re: markup/etc.

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). Internationalization (i18n) Issues or PRs related to internationalization efforts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants