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

fix: Improve empty block text #11560

Merged
merged 8 commits into from
Nov 8, 2018
Merged

Conversation

tofumatt
Copy link
Member

@tofumatt tofumatt commented Nov 7, 2018

Fix #9076
Fix #5591

Consolidate the "Write your story" and "Add text or type" placeholders, while also adding context to the placeholder with an aria-label.

Screenshots

screenshot 2018-11-07 02 57 05

screenshot 2018-11-07 02 56 48

@tofumatt tofumatt added this to the 4.3 milestone Nov 7, 2018
@tofumatt tofumatt requested a review from a team November 7, 2018 02:55
@@ -245,7 +245,8 @@ class ParagraphBlock extends Component {
onMerge={ mergeBlocks }
onReplace={ this.onReplace }
onRemove={ () => onReplace( [] ) }
placeholder={ placeholder || __( 'Add text or type / to add content' ) }
aria-label={ __( 'Type text or use the forward slash key to insert a block' ) }
placeholder={ placeholder || __( 'Type text or press “/” to select a block' ) }
Copy link
Member

@chrisvanpatten chrisvanpatten Nov 7, 2018

Choose a reason for hiding this comment

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

It's a super minor nit pick but why can't these labels be identical, with the aria label imply replacing "/" with "forward slash key" so it's clearer how screen readers should read it?

E.g.

Type text or press the forward slash key to insert a block

and

Type text or press the "/" key to insert a block

It's not clear why the text itself would need to be different in these cases, aside from the visible placeholder showing the slash visually instead of as text.

Copy link
Member Author

Choose a reason for hiding this comment

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

Works for me actually, good point 👍

<WithSelect(WithDispatch(InserterWithShortcuts)) />
<WithSelect(IfCondition(Inserter))
position="top right"
/>
Copy link
Contributor

Choose a reason for hiding this comment

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

Anything that might have triggered all these tabs? I guess jest generates spaces by default right?

Copy link
Member Author

Choose a reason for hiding this comment

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

I just typed u after running test-unit:watch to update the snapshots, so I blame Jest. Should I have run something else?

Copy link
Contributor

Choose a reason for hiding this comment

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

No, if it's generated by jest this way than maybe the old snapshot was not correct.

@@ -80,7 +80,7 @@ export class PostTextEditor extends Component {
onBlur={ this.stopEditing }
className="editor-post-text-editor"
id={ `post-content-${ instanceId }` }
placeholder={ decodedPlaceholder || __( 'Write your story' ) }
placeholder={ decodedPlaceholder || __( 'Type text or press “/” to insert a block' ) }
Copy link
Contributor

Choose a reason for hiding this comment

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

You can't press "/" or insert a block. This is the text editor, you can just type HTML :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, oops, right. I think it too had the “Write your story” prompt and I just got confused. I’ll update it, my bad! 😅

Copy link
Member Author

Choose a reason for hiding this comment

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

This will actually still show because we pass the same placeholder to the code editor. We probably shouldn't do that, but it's a different issue...

Copy link
Contributor

Choose a reason for hiding this comment

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

It's made visible with this PR though, I'd be in favor of dropping the bodyPlaceholder support here.

@youknowriad
Copy link
Contributor

This is looking good to me, but I prefer to bring people that I know care more than me about "Copy". cc @jasmussen @mtias

@jasmussen
Copy link
Contributor

👍 👍 consolidation, 👍 👍 PR.

However I prefer the suggestion from the old PR, which was: Start writing or type / to choose a block.

@tofumatt
Copy link
Member Author

tofumatt commented Nov 7, 2018

However I prefer the suggestion from the old PR, which was: Start writing or type / to choose a block.

Actually, I agree. Changed!

@@ -246,7 +246,7 @@ class ParagraphBlock extends Component {
onReplace={ this.onReplace }
onRemove={ () => onReplace( [] ) }
aria-label={ __( 'Empty block; type text or press the forward slash key to insert a block' ) }
Copy link
Member

Choose a reason for hiding this comment

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

should "type text" be replaced with "start writing"?

Copy link
Member Author

Choose a reason for hiding this comment

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

For a screen reader I actually think "type text" is better than "start writing".

@@ -71,7 +71,7 @@ export class PostTextEditor extends Component {
return (
<Fragment>
<label htmlFor={ `post-content-${ instanceId }` } className="screen-reader-text">
{ decodedPlaceholder || __( 'Write your story' ) }
{ decodedPlaceholder || __( 'Type text' ) }
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's remove the decodedPlaceholder from this component and ship.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds good; if that means this is otherwise good to go mind explicitly reviewing it now? 😄

Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@youknowriad
Copy link
Contributor

(Looks like there's some failing tests though)

@tofumatt
Copy link
Member Author

tofumatt commented Nov 8, 2018

Shoot, must have run the wrong snapshot update script. Fixed.

@tofumatt tofumatt merged commit ddf8c07 into master Nov 8, 2018
@tofumatt tofumatt deleted the fix/5591-improve-new-block-text branch November 8, 2018 16:28
@mtias mtias added [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Feature] Writing Flow Block selection, navigation, splitting, merging, deletion... [Type] Copy Issues or PRs that need copy editing assistance labels Nov 8, 2018
@afercia
Copy link
Contributor

afercia commented Nov 8, 2018

@tofumatt this way, it will announce "Empty block" even if the block is not empty... can this be reviewed please? Also, changes in this area should take into consideration #1659. Thank you.

screenshot 2018-11-08 at 19 21 10

@tofumatt
Copy link
Member Author

tofumatt commented Nov 8, 2018

Argh, right, sorry. Okay, I'll file a follow-up PR for this tonight. Thanks so much for that @afercia, my bad. 😓

@jasmussen
Copy link
Contributor

jasmussen commented Nov 9, 2018

Thanks for working on this, and apologies for missing this. But right now the text says

Start writing or press / to insert a block

The suggestion was:

Start writing or type / to choose a block

I defer to any other considerations, but I honestly prefer the initially suggested phrasing. Notaby "press /" feels weird to me, because on a european keyboard this is not a separate key — I have to press Shift + 7 to insert a /. Therefore "type" feels more natural.

Additionally, I'm not a fan of the term "insert" for blocks, even though it's accurate. I far prefer "add", or as was suggested "choose a block". The latter implies your caret is already in a block, and you can change the type of it, which is sort of accurate as anything you add replaces the appender block you were in.

youknowriad pushed a commit that referenced this pull request Nov 9, 2018
* fix: Only show aria-label when content is empty

Fix issue introduced in #11560

* Improve aria labels so they reflect state of block/empty block
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Writing Flow Block selection, navigation, splitting, merging, deletion... [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Type] Copy Issues or PRs that need copy editing assistance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants