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

Allow for classes, rowspan, colspan and attributes on row headers #1367

Merged
merged 4 commits into from
May 22, 2019

Conversation

36degrees
Copy link
Contributor

@36degrees 36degrees commented May 21, 2019

This change allows you to pass classes, rowspan, colspan and attributes to the first cell in each row when firstCellIsHeader is true.

It also simplifies the logic of the table template, and tidies up the tests to use a single assertion per test, improve coverage and use a consistent format. (Split into a future PR)

Fixes #1261

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1367 May 21, 2019 13:45 Inactive
@36degrees 36degrees force-pushed the table-improvements branch from 84060f2 to cfb673d Compare May 21, 2019 13:46
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1367 May 21, 2019 13:46 Inactive
Copy link
Contributor

@NickColley NickColley left a comment

Choose a reason for hiding this comment

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

Nice fix with some good cleanup 👍

Test restructuring is very difficult to review, but I'm assuming they passed before you restructured them? In future might be better to do that kind of refactoring separately.

@36degrees
Copy link
Contributor Author

Happy to split the last two commits out into a separate PR if you prefer?

@NickColley
Copy link
Contributor

I think it's up to you depending on how confident you are about the new tests.

@36degrees 36degrees force-pushed the table-improvements branch from cfb673d to 03550eb Compare May 22, 2019 15:08
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1367 May 22, 2019 15:08 Inactive
@36degrees
Copy link
Contributor Author

👍 I've split that work out and will raise another PR once this has been merged.

@36degrees 36degrees force-pushed the table-improvements branch from 03550eb to 3e3351a Compare May 22, 2019 15:12
36degrees added 4 commits May 22, 2019 16:12
We used to append a scope="row" to the first table cell in a row, which we did by branching on whether loop.first was true. This was later removed (as scope is only valid on td elements) but the logic was not.
@36degrees 36degrees changed the title Allow for classes, rowspan, colspan and attributes on row headers; tidy up tests Allow for classes, rowspan, colspan and attributes on row headers May 22, 2019
@36degrees 36degrees merged commit 39e11ca into v3 May 22, 2019
@36degrees 36degrees deleted the table-improvements branch May 22, 2019 15:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

3 participants