Conversation
| {%- endif %} | ||
|
|
||
| {%- if params.responsive %} | ||
| <figure class="nhsuk-table__list-view-container"> |
There was a problem hiding this comment.
Because list-view-container isn't a child of nhs-table can we rename this?
| <figure class="nhsuk-table__list-view-container"> | |
| <figure class="nhsuk-table-list-view-container"> |
There was a problem hiding this comment.
nhsuk-table__panel-with-heading-tab and nhsuk-table__heading-tab aren't children of nhsuk-table either. In practice the rule seems to be that the nhsuk-table is the block that implements the primary view, and auxiliary but semantically linked elements share that prefix. I don't think that's unreasonable, because if we were constrained to encoding a hierarchy we'd just chain the classes.
There was a problem hiding this comment.
Yeah, they're overdue a refactor too 😬
Just pointing it out because we follow BEM methodology (and BEM for everyone else)
We have an unused .nhsuk-table-container in the CSS so would be a good opportunity for a reset
.nhsuk-table-container {}
.nhsuk-table-container__panel {}
.nhsuk-table-container__tab {}
// etcThere was a problem hiding this comment.
i.e. We have multiple blocks, with a block (container) being a container for another block (table)
There was a problem hiding this comment.
Yes, I'm familiar with BEM. I didn't think it required a parent-child relationship, just that the elements aren't used separately. If we're insisting on containment that's fine.
There was a problem hiding this comment.
Sorry I should have said child descendant
Yeah we should use this opportunity to go for containment, thanks Alex
| {%- if params.responsive %} | ||
| <figure class="nhsuk-table__list-view-container"> | ||
| {%- if params.caption %} | ||
| <figcaption class="nhsuk-table__caption--m |
There was a problem hiding this comment.
Gets a bit wordy though, but preserves ${block}__${element}--${modifier}
See what else works
Edit: It was this comment that has the suggestion that was a bit wordy but I'm not totally sure how to address the nhsuk-table__caption element existing outside of its nhsuk-table block 😔
There was a problem hiding this comment.
This one is specifically to reuse the already-defined style so we don't have to duplicate it. The figcaption doesn't need its own class here.
There was a problem hiding this comment.
I know it doesn't need it, but the pattern is to add one
Would be brilliant to refactor and share nhsuk-table__caption styles where possible though
Then we can tap into these:
.nhsuk-table__caption--s
.nhsuk-table__caption--m
.nhsuk-table__caption--l
.nhsuk-table__caption--xlSticking with BEM allows us to refactor elements or nesting in future, without breaking changes
But how do we follow the convention when the <figcaption> isn't a child of nhsuk-table?
There was a problem hiding this comment.
The fundamental problem here is that nhsuk-table is on the wrong DOM element. There should be a root div to wrap all the related gubbins that goes outside the <table> itself. That's not something I'm particularly keen to pick up as part of this PR though.
There was a problem hiding this comment.
I caught up with the team and they're happy for me to help with this?
We've got other breaking changes coming up (e.g. updated header) so would be the perfect opportunity
ccbc4a1 to
b37d86e
Compare
|
@regularfry I've rebased this to resolve any recent conflicts I'll be coming back to pick up from this comment soon |
|
I've updated the blank cells example to extend the new Plus missing reference screenshots |
58adb89 to
b6382f2
Compare
31093dd to
6e9fe8c
Compare
|
Rebased again into new packages structure |
b1db795 to
b594883
Compare
b594883 to
dbcc2a6
Compare
|
I'm going to rebase this PR branch @anandamaryon1 and have a look later if I can 😬
|
fbaaabd to
0df0524
Compare
|




Description
This patch is an implementation of the markup-swapping fix for #1092 . It introduces a few new elements: since the
tableuses acaptionand we need to preserve that, butcaptionisn't valid outside atable, thedlimplementation usesfigureandfigcaptionto do the same job.Tested so far on macOS Chrome and Safari, and the VoiceOver/Safari combination.
Also fixes #1123.
Checklist