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

Try accessible excerpt #3815

Merged
merged 3 commits into from Dec 12, 2017

Conversation

Projects
None yet
4 participants
@jasmussen
Contributor

jasmussen commented Dec 5, 2017

This is an attempt to fix #1371.

CC: @afercia, does this address the fundamental issue?

Screenshot:

screen shot 2017-12-05 at 12 15 10

Rendered markup:

<label for="editor-post-excerpt">Write an excerpt (optional)</label>
<textarea class="editor-post-excerpt__textarea" aria-label="Excerpt" id="editor-post-excerpt"></textarea>
Try accessible excerpt
This is an attempt to fix #1371.

CC: @afercia, does this address the fundamental issue?

@jasmussen jasmussen self-assigned this Dec 5, 2017

@jasmussen jasmussen requested review from youknowriad and afercia Dec 5, 2017

@afercia

This comment has been minimized.

Show comment
Hide comment
@afercia

afercia Dec 5, 2017

Contributor

@jasmussen thanks. From an accessibility perspective, the new <label> element is good, we should remove the aria-label="Excerpt" though.

Contributor

afercia commented Dec 5, 2017

@jasmussen thanks. From an accessibility perspective, the new <label> element is good, we should remove the aria-label="Excerpt" though.

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Dec 6, 2017

Contributor

Thanks so much for the reviews, both. I've updated to use withInstance to add a unique ID, and I've removed the aria-label.

Contributor

jasmussen commented Dec 6, 2017

Thanks so much for the reviews, both. I've updated to use withInstance to add a unique ID, and I've removed the aria-label.

@jorgefilipecosta

jorgefilipecosta approved these changes Dec 8, 2017 edited

Code changes look good to me, I just left minor non-blocking comments. I did not found any regression during my tests, provided the design changes are ok and the a11y issue is fixed this is good to go.

@@ -46,5 +47,4 @@ export default connect(
return editPost( { excerpt } );
},
}
)( PostExcerpt );
)( withInstanceId( PostExcerpt ) );

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Dec 8, 2017

Member

Minor: Normally we use compose in this cases. (the syntax referred by @aduth)

@jorgefilipecosta

jorgefilipecosta Dec 8, 2017

Member

Minor: Normally we use compose in this cases. (the syntax referred by @aduth)

@aduth

aduth approved these changes Dec 11, 2017

@jasmussen jasmussen merged commit c1a0a9f into master Dec 12, 2017

3 checks passed

codecov/project 39.84% (+2.07%) compared to 1dbc27d
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@jasmussen jasmussen deleted the try/accessible-excerpt branch Dec 12, 2017

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