-
-
Notifications
You must be signed in to change notification settings - Fork 142
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
Improve table layout #288
Improve table layout #288
Conversation
|
||
th, | ||
td { | ||
padding: $small-spacing $small-spacing $small-spacing 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shorthands of length 4
are not allowed. Value was $small-spacing $small-spacing $small-spacing 0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is bad guidance. The best way to achieve this padding
is via that shorthand (versus long-hand of margin-top
, margin-right
, etc.). Just because I need 0
for margin-left
doesn’t mean I should be throwing off our linter. I’ve adjusted scss-lint (and Hound in return) in ca1aea7.
👍 |
contrib/index.html
Outdated
<td>Division 1</td> | ||
<td> | ||
Division 2 with some even longer text that seems to just ramble on | ||
and on… |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we have a proper ellipsis? …
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small comment. Otherwise ⛵️
- Increase bottom and top margin on tables - This matches the same bottom and top margin values we set on `hr` elements. - Move `text-align` property to `table` - In most situations, left-aligned text within a table is ideal. This makes sure the whole table is left-aligned, while still allowing easy overriding on a per-column basis (e.g. for numeral data). - Change vertical alignment of tables - Vertically aligning cell text in the middle makes for a bad reading experience because there is no consistent or expected starting point for text that the eyes can pick up on. - This changes the vertical alignment of cell text to: - Bottom of the cell in the table header (`thead`) - Top of the cell in the table body (`tbody`) - Set table borders on the row - Rather than setting borders on each individual cell, this sets it on the whole row (`tr`). - Use the headings line-height in table headers - Add padding to the right of each table cell - This prevents adjacement cell text from butting into each other.
I’ve been working on a few table-heavy projects lately and I’ve found that the base table styles that Bitters provided me left a lot to be desired. Specifically, if the table has decently long strings of text within it, the layout really fell apart and made for a painful reading experience. This PR updates the table styles with a new niceties which I think makes a great starting point for nearly all tables, and also updates the contributing page markup to include some longer text within the cells.
hr
elements.text-align
property totable
thead
)tbody
)tr
).Before
After