-
Notifications
You must be signed in to change notification settings - Fork 67
Tables: Table styling #151
base: main
Are you sure you want to change the base?
Conversation
Thank you for your pull request. It looks like this may be your first contribution to a jQuery Foundation project, if so we need you to sign our Contributor License Agreement (CLA). 📝 Please visit http://contribute.jquery.org/CLA/ to sign. After you signed, the PR is checked again automatically after a minute. If there's still an issue, please reply here to let us know. If you've already signed our CLA, it's possible your git author information doesn't match your CLA signature (both your name and email have to match), for more information, check the status of your CLA check. |
scss/atoms/tables/_tables.scss
Outdated
} | ||
|
||
.striped-table > tbody > tr:nth-of-type(odd), { | ||
background-color: #f2f2f2; |
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.
.table--striped maybe? considering other naming convention you used( like .table--responsive)
@kristyjy , could use more variables, especially with colors. |
scss/atoms/tables/_tables.scss
Outdated
border-top: map-get($table-base, border); | ||
padding: map-get($table-base, padding); | ||
color: map-get($table-base, color); | ||
font-weight: 400; |
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.
Use a variable?
|
scss/atoms/tables/_tables.scss
Outdated
table { | ||
width: 100%; | ||
margin: map-get($table-base, margin); | ||
font-size: map-get($table-base, font-size); |
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.
consider using em function on this (See https://github.com/jquery/css-chassis/blob/master/scss/atoms/typography/_functions.scss )
@geekman-rohit @kristyjy At one point the CLA was valid. Not sure why it isn't anymore: #130 |
7938c90
to
6f0fa30
Compare
Do we have a full width table option? |
Tables can currently be viewed up in: http://view.css-chassis.com/53-tablestyles/demos/tables.html Based on a discussions in today's meetings: |
One thing to consider is the effect on nested tables, which the designer might want to style differently from the ancestor table, so it's bad if the styles "bleed" into descendant tables. Bootstrap got complaints about this. |
demos/tables.html
Outdated
<head> | ||
<meta charset="utf-8"> | ||
<title>CSS Chassis - Tables</title> | ||
<meta name="description" content="Typography skeleton for styling"> |
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.
content= "Typography..." -> content="Table..." I keep missing this too
scss/variables/tables.js
Outdated
"margin": "0 0 1em", | ||
"font-size": "16px", | ||
"thead-font-size": "12px", | ||
"border": "1px solid #eee", |
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.
Would be better to pull color (#eee) from the color pallette. "default-dark" is #eee
3a19537
to
826b1b3
Compare
|
This is a WIP pull request looking for feedback. I'd like to discuss if we should style the element like in this PR or make classes like the buttons PR did either here or in the meeting tomorrow.