Typography: Added basic styles for paragraphs and lists #142
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. |
|
@thejdeep please sign our CLA at http://contribute.jquery.org/CLA/ |
|
Done. I am waiting for it to re-check. |
|
@geekman-rohit Shouldn't be taking this long ? |
|
@thejdeep the bot doesn't update automatically, it will update the next time you push a commit . If its time to pull in ur PR and the label is not updated, I'll confirm with the team that you have signed and update the label manually. |
|
@geekman-rohit the CLA does recheck automatically once a minute or so this is an actual failure. @thejdeep the problem is you need to sign with your full name ( which i see you did at first ) but the name in your git config is just your first name and thats what its matching and checking against. It finds a match but fails on it only having first name. It should say this in the error details but looks like its returning a 404 on the link sorry about that! |
thejdeep
force-pushed
the
typo
branch
3 times, most recently
from
c295fac to
14a3d8f
Compare
|
@thejdeep you'll have to pull in latest commits and rebase to resolve conflicts |
|
|
||
| @mixin paragraph($style) { | ||
| text-align: map-get($style,textAlignment); | ||
| margin: em(map-get($style,margin)) 0; |
There was a problem hiding this comment.
This mixin is called only once, do we really need it as a mixin?
| value: { | ||
| margin: "25px", | ||
| fontSize: "16px", | ||
| textAlignment: "justify" |
There was a problem hiding this comment.
Not sure I would go with justify here. for the basic <p> styling.
thejdeep
force-pushed
the
typo
branch
2 times, most recently
from
8cfb430 to
bf6c02a
Compare
| ol ul, | ||
| ul ol, | ||
| ol ol { | ||
| margin: 0; |
There was a problem hiding this comment.
still something wrong with the indentation. No spaces. Single tab. Turn off soft tabs in your text editor.
thejdeep
force-pushed
the
typo
branch
2 times, most recently
from
6dc25ed to
c512ef3
Compare
| text-align: map-get($p,textAlignment); | ||
| margin: em(map-get($p,marginTopBottom)) em(map-get($p,marginLeftRight)); | ||
| line-height: 1.5; | ||
| font-size: map-get($defaultFont, font-size); |
There was a problem hiding this comment.
$defaultFont doesnot exist anymore. Its $font-default now.
check https://github.com/jquery/css-chassis/blob/master/scss/variables/typography.js
Rebase this branch on jquery/master to see. That will also remove conflicts.
| capitalization: "uppercase" | ||
| } | ||
| }, | ||
| p: { |
There was a problem hiding this comment.
as discussed in meeting on tuesday. Double quotes around scss variables defined in jsass
check my comment on : #135
Typography: Eliminated need of a mixin for p Typography: Eliminated need of mixin for p Typography: Shorthand margins Typography: Eliminated need of mixin for p Typography: Fixed indentation Typography: Fixed default fontSize Typography: Changed to follow naming conventions
|
Is there a visual demo of paragraph and list tags? |
| text-align: map-get($p,text-alignment); | ||
| margin: em(map-get($p,margin-top)) em(map-get($p,margin-right)) em(map-get($p,margin-bottom)) em(map-get($p,margin-left)); | ||
| line-height: 1.5; | ||
| font-size: map-get($font-default, font-size); |
There was a problem hiding this comment.
You are assigning the default font size to p directly. That will happen anyways as p will inherit the style.
Instead you should have a font-size variable in $p map which is set to the default font size by default, and user can overwrite if he wants
There was a problem hiding this comment.
use em function on this so we get value in em
| } | ||
|
|
||
| ul, ol { | ||
| margin: 0 0 10px; |
There was a problem hiding this comment.
margins should probably be put into a variable
There was a problem hiding this comment.
also just styling for ol/ul and nested ol/ul? no li styling?
|
Will work on this over the next few days. Was swamped with finals. |
|
Any updates? |
No description provided.