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

TAI-1139 added table--heavy-header #394

Merged
merged 1 commit into from
Feb 10, 2016
Merged

Conversation

timsb
Copy link
Contributor

@timsb timsb commented Feb 8, 2016

Added a variant for child of table element with a blue border.
table--heavy-header


.table--heavy-header {
border-bottom: em(2) solid $govuk-blue-colour;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand the use of the name heavy, is it the border or colour?
So given this is on the th element and there are no other header selectors I would be tempted to use:
.table-header
or
.table__header
Then if there are different iterations in the future we can add modifiers then.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is the default th styling not in effect the table__header? I would have thought that this is a variant of the default th? .table__header--heavy?

Copy link
Contributor

Choose a reason for hiding this comment

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

I suppose this is a broader question for designers what is the default border styling for a table?
The work on the th should be a reset in base. The component should have a standard border size.

.table__header {
border-bottom: 2px solid;
border-color: <default>
}

Then we have a tones file

.tone-border-govuk {
border-colour: $govukBlue;
}

Implementation

<th class="table__header tone-border-govuk">

But that doesn't help the current situation!

Is the default th styling not in effect the table__header? I would have thought that this is a variant of the default th? .table__header--heavy?

Potentially .table__header--highlight although this feels a little odd to me without a .table__header class.
As tables are a guaranteed structure in html you get the table styles for free when adding a table without a selector, apply .table__header and you get the custom styles, this would be my ideal.

Or following the same patten then:

.table--header-highlight {
   th {
      border-bottom: em(2) solid $govuk-blue-colour;
   }
}

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 like the:

.table--header-highlight {
th {
border-bottom: em(2) solid $govuk-blue-colour;
}
}

but shouldn't we try to keep the class as close as possible to the component that is being affected i.e. the th?

So it would be .table__header--highlight

TAI-1139 renamed class to table__header--highlight
@feedmypixel
Copy link
Contributor

LGTM - please see discussion in comments above and issue opened when refactoring the tables work 👍

feedmypixel added a commit that referenced this pull request Feb 10, 2016
TAI-1139 added table--heavy-header
@feedmypixel feedmypixel merged commit 9ebe2f0 into hmrc:master Feb 10, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants