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

Adding scope to all table headers, fix #12401 #15079

Merged
merged 4 commits into from
Nov 21, 2017

Conversation

timroes
Copy link
Contributor

@timroes timroes commented Nov 21, 2017

This PR adds the scope attribute to all th elements where it makes sense. We still have a very few places left, we are using a table actually for styling purposes and not as a table, I haven't touched these places.

It also adds the scope property for the KuiTableHeaderCell UI framework component.

@timroes
Copy link
Contributor Author

timroes commented Nov 21, 2017

Jenins, test this

@timroes
Copy link
Contributor Author

timroes commented Nov 21, 2017

Jenkins, test this

Copy link
Member

@ppisljar ppisljar left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@cjcenizal cjcenizal left a comment

Choose a reason for hiding this comment

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

Awesome! I love the changes to the UI Framework. Thanks for doing that. I have one quick question about colgroup before I give a 👍 .

@@ -33,7 +33,7 @@
<table ng-if="columns" class="table agg-table-group">
<thead>
<tr>
<th ng-repeat="table in columns" ng-if="table.tables">
<th ng-repeat="table in columns" ng-if="table.tables" scope="colgroup">
Copy link
Contributor

Choose a reason for hiding this comment

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

What was your reasoning behind choosing colgroup in this file? The W3 page mentions this:

before making these associations [with scope="colgroup"], the structure of such groups of columns and rows needs to be defined in the table markup:

  • A column group is defined using the element

My naive interpretation of this is that we shouldn't use scope="colgroup" unless there's a <colgroup> element.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would agree with you, that this is wrong here. Also looking at the HTML again, I think I just misread it, so will fix this.

Copy link
Contributor

@cjcenizal cjcenizal left a comment

Choose a reason for hiding this comment

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

LGTM!

@timroes timroes merged commit 6e1faea into elastic:master Nov 21, 2017
@timroes timroes deleted the a11y-tables branch November 21, 2017 19:49
timroes added a commit to timroes/kibana that referenced this pull request Nov 21, 2017
* Add scope for all th in Kibana

* Add scope to <th> in ui framework

* Update jest snapshots with scope

* Fix wrong usage of colgroup
timroes added a commit that referenced this pull request Nov 21, 2017
* Add scope for all th in Kibana

* Add scope to <th> in ui framework

* Update jest snapshots with scope

* Fix wrong usage of colgroup
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants