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.

Sign up for GitHub

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

Jump to bottom

Components: Use a div wrapper for the toolbar #3001

Merged
merged 1 commit into from
Oct 13, 2017
Merged

Components: Use a div wrapper for the toolbar #3001

merged 1 commit into from
Oct 13, 2017

Conversation

youknowriad
Copy link
Contributor

Per comment #2960 (comment) The toolbar wrapper shouldn't be a list, it was also creating some invalid HTML in some places where we were using the Toolbar component without using li as children.

Testing instructions

  • Check that the toolbars still show up properly (Block toolbars)

@youknowriad youknowriad added the [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). label Oct 12, 2017
@youknowriad youknowriad self-assigned this Oct 12, 2017
@codecov
Copy link

codecov bot commented Oct 12, 2017 •
edited
Loading

Codecov Report

Merging #3001 into master will increase coverage by 0.87%.
The diff coverage is 25%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3001      +/-   ##
==========================================
+ Coverage   34.37%   35.25%   +0.87%     
==========================================
  Files         196      196              
  Lines        5809     6192     +383     
  Branches     1027     1117      +90     
==========================================
+ Hits         1997     2183     +186     
- Misses       3222     3376     +154     
- Partials      590      633      +43
Impacted Files Coverage Δ
blocks/library/audio/index.js 12.12% <ø> (ø) ⬆️
blocks/library/video/index.js 21.05% <ø> (ø) ⬆️
blocks/library/image/block.js 0% <ø> (ø) ⬆️
blocks/library/cover-image/index.js 33.33% <ø> (ø) ⬆️
blocks/url-input/button.js 0% <ø> (ø) ⬆️
blocks/library/gallery/block.js 11.11% <0%> (ø) ⬆️
blocks/library/table/table-block.js 8% <0%> (ø) ⬆️
components/toolbar/index.js 100% <100%> (ø) ⬆️
editor/block-mover/index.js 0% <0%> (ø) ⬆️
editor/modes/visual-editor/block-list.js 0% <0%> (ø) ⬆️
... and 13 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3163539...e4feb9d. Read the comment docs.

@afercia
Copy link
Contributor

afercia commented Oct 12, 2017

Fine for me, thanks! From an accessibility perspective, the toolbar wrapper should have a role="toolbar" but... the problem is toolbars are actually composed by multiple "toolbar" components that change depending on the context. Instead, there should be just one wrapper with role="toolbar" and just controls as first level children. I understand this would be a major change and should be addressed separately, probably on #2148.

@afercia afercia mentioned this pull request Oct 12, 2017
Merged
7 tasks
aduth
aduth reviewed Oct 12, 2017
View reviewed changes
editor/block-switcher/style.scss Outdated
@@ -5,6 +5,7 @@
font-size: $default-font-size;
line-height: $default-line-height;
margin-right: -1px;
margin-bottom: -1px;
Copy link
Member

Choose a reason for hiding this comment

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

Can you explain this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This fixed a double border issue, but it looks like it's unnecessary after updating the remaining ul references.

aduth
aduth previously requested changes Oct 12, 2017
View reviewed changes
Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

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

There are two more ul.components-toolbar styles in editor/sidebar/style.scss.

components/toolbar/style.scss Outdated
@@ -6,14 +6,12 @@
}

ul.components-toolbar {
Copy link
Member

Choose a reason for hiding this comment

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

This element selector needs to be dropped? Or changed to div?

@youknowriad youknowriad dismissed aduth’s stale review October 12, 2017 18:00

Remaning ul mentions updated

gziolo
gziolo reviewed Oct 13, 2017
View reviewed changes
blocks/url-input/button.js
@@ -40,7 +40,7 @@ class UrlInputButton extends Component {
const { expanded } = this.state;

return (
<li className="blocks-url-input__button">
<div className="blocks-url-input__button">
Copy link
Member

@gziolo gziolo Oct 13, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I double checked and it looks like it is only used in the toolbar at the moment. It makes a lot of sense to use div here as it makes it more reusable 👍

gziolo
gziolo reviewed Oct 13, 2017
View reviewed changes
components/toolbar/test/index.js
@@ -33,7 +33,6 @@ describe( 'Toolbar', () => {
];
const toolbar = shallow( <Toolbar controls={ controls } /> );
const listItem = toolbar.find( 'IconButton' );
Copy link
Member

Choose a reason for hiding this comment

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

I found this jest-enzyme yesterday. It might simplify testing components. We can also experiment with snaphsots to minimize the number of assertions where we validate HTML, which should be an implementation detail that is more likely to change.

gziolo
gziolo approved these changes Oct 13, 2017
View reviewed changes
Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

Code looks very good, tests well 👍

@youknowriad youknowriad merged commit ced2161 into master Oct 13, 2017
@youknowriad youknowriad deleted the try/remove-toolbar-li branch October 13, 2017 09:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gziolo gziolo gziolo approved these changes

@afercia afercia afercia approved these changes

@aduth aduth aduth left review comments

Assignees

@youknowriad youknowriad

Labels
[Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes).
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@youknowriad @afercia @gziolo @aduth