Improve accessibility: use ng-repeat on <tr> instead of <tbody> #1045
Conversation
| > tr { | ||
| + tr { | ||
| &:nth-child(2) { | ||
| border-top-width: 0; |
There was a problem hiding this comment.
@rhamilto I was not sure if this was the correct approach to fixing the double border on tables that need a hidden <tr> before the actual first visible <tr> element. wdyt?
There was a problem hiding this comment.
I think @sg00dwin is probably a better qualified as I know he has recently fixed some stuff in _tables.less.
juanvallejo
force-pushed
the
jvallejo/move-ng-repeat-from-tbody-to-tr
branch
from
b857d10 to
bde4c28
Compare
| border-top-width: 0; | ||
| } | ||
| > tr.nested-loop ~ tr.nested-loop { | ||
| border-top-width: 2px; |
There was a problem hiding this comment.
@sg00dwin @jwforres @spadgett
This works for all cases where there must be sibling elements (with the first one set to display: none;) in order to have nested loops (traffic-table.html and deployments.html in this PR). I first remove the border from all <tr> elements in the loop, and then re-apply it to every <tr> in that loop except the first one by using tr.nested-loop ~ tr.nested-loop.
There was a problem hiding this comment.
I guess I'm not clear on why there is a top border on the first hidden row. If display: none, shouldn't the border be hidden as well?
There was a problem hiding this comment.
The border comes from the first non-hidden <tr> (with className nested-loop) (see attached image), but normally, this first non-hidden <tr> would not have a border-top-width due to the css rule tbody > tr + tr {... which would only apply the border to <tr> tags that immediately follow other <tr> tags. Normally the top 2px border that is seen, is the overall <table> element's top-border. Because the hidden <tr> right above the one in the image still exists in the dom, it forces this first visible <tr> to have a border-top-width, which combined with the table's existing 2px results in the top border with extra width that we are currently seeing in these tables.
To get around this, I select all non-hidden <tr> elements by adding the class nested-loop to them, remove their border-top-width, and then re-apply it to every one except the first .nested-loop tr.
juanvallejo
force-pushed
the
jvallejo/move-ng-repeat-from-tbody-to-tr
branch
from
bde4c28 to
5009a3a
Compare
|
Does it work if we change the |
I have tested this on both imagestream.html, and deployments.html and it only seems to work in |
juanvallejo
force-pushed
the
jvallejo/move-ng-repeat-from-tbody-to-tr
branch
from
58a95e4 to
2a75699
Compare
There was a problem hiding this comment.
@juanvallejo looks good, please squash your commits and I'll merge
This patch repeats table rows rather than table bodies for any list of data displayed in a table. It resolves the issue of a double top-border on tables when there are nested ng-repeat loops. (In these cases, a double top border was appearing due to the need for a sibling <tr> element to contain the outer loop. This extra element caused the css rule "tbody > tr + tr" to apply a top-border to the first visible <tr> element as well, making the table's already existing top-border redundant). This patch removes the `<table>` element's top border and adds it instead to all nested `<tr>` elements in mobile view.
juanvallejo
force-pushed
the
jvallejo/move-ng-repeat-from-tbody-to-tr
branch
from
2a75699 to
0874897
Compare
Thanks! done |
|
[merge] |
|
Evaluated for origin web console merge up to 0874897 |
|
Origin Web Console Merge Results: SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin_web_console/880/) (Base Commit: e66c5ad) |
This patch repeats table rows rather than table bodies for any list of
data displayed in a table.
It also resolves the issue of a double top-border on tables when there are
nested ng-repeat loops. (In these cases, a double top border was
appearing due to the need for a sibling element to contain the
outer loop. This extra element caused the css rule "tbody > tr + tr" to
apply a top-border to the first visible element as well, making the
table's already existing top-border redundant).
@spadgett @jwforres @rhamilto @sg00dwin