-
-
Notifications
You must be signed in to change notification settings - Fork 555
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Issue#934 #1565
base: main
Are you sure you want to change the base?
Issue#934 #1565
Conversation
We have tried fixing the following errors: 10:49:28 AM: 12:12 ✖ Expected newline after ":" with a multi-line declaration declaration-colon-newline-after And leave formatting as you had it previously. However, they still appear in our terminal. Plus, we didn't do the npm run publish because we didn´t want to deploy. We were just tring to create unified variables for margins and paddings. Hope this helps you! |
@ttkapostol Thanks for the work on this. Those errors are minor but are preventing the site from building a preview. I think it's maybe an issue with character encodings? Or a formatter gone wild. I'll have a fix for this in a bit that hopefully let's the site build. Are you on Windows? |
Thank you very much @davatron5000 ! |
@davatron5000 are you going to review this? Else I am happy to review it some time around end of this week. |
@SaptakS If you don't mind taking a look. I'll try to review it as well. Sorry this has taken so long. So far it looks good to me. Pretty straight forward. |
Will do. |
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.
Apologies for the delay in reviewing. This PR looks great! I have left few inline comments on lines I think were maybe mistakenly left from a previous iteration and hasn't been updated? Once those are addressed, I think this PR should be good to be merged.
.c-footer__list { | ||
@include preserve-list-semantics(); | ||
|
||
margin-top: 1rem; | ||
margin-top: $m-3; |
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.
I think this should be $m-2
margin-left: 0.4rem; | ||
|
||
// Text-level formatting | ||
li { | ||
margin-top: 0.5rem; | ||
margin-top: $m-2; |
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.
I think this should be $m-1
.c-homepage-card__description { | ||
font-family: $font-family-secondary; | ||
margin-top: 1rem; | ||
margin-top: $m-3; |
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.
I think this should be $m-2
@@ -74,7 +66,7 @@ | |||
@include var(border-color, secondary-link-accent); | |||
@include var(color, secondary-link-text); | |||
font-family: $font-family-secondary; | |||
margin-top: 1rem; | |||
margin-top: $m-3; |
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.
I think this should be $m-2
.c-linkroll__category { | ||
@include var(color, card-meta-text); | ||
} | ||
|
||
|
||
.c-linkroll__category--featured { | ||
padding-top: 1rem; |
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.
Why not update this?
} | ||
|
||
.u-spacing-left-longer { | ||
margin-left: 2rem; | ||
margin-left: $m-4; | ||
} | ||
|
||
.u-spacing-left-longest { | ||
margin-left: 3rem; |
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.
I think this should be $m-6
} | ||
|
||
.u-spacing-bottom-short { | ||
margin-bottom: 0.75rem; | ||
} | ||
|
||
.u-spacing-bottom-medium { | ||
margin-bottom: 1rem; | ||
margin-bottom: $m-2; | ||
} | ||
|
||
.u-spacing-bottom-long { | ||
margin-bottom: 1.5rem; |
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.
I think this should be $m-3?
Ideally, I would also want to reduce the number of variables and maybe normalize some of the spacing to avoid having to need so many different variables |
$m-9: 4.5rem; | ||
$m-10: 5rem; | ||
$m-11: 5.5rem; | ||
$m-12: 6rem; |
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.
I was also thinking if we could have something more like:
$m-0: 0;
$m-xxs: 0.5rem;
$m-xs: 1rem;
$m-s: 1.5rem;
$m-m: 2rem;
$m-l: 3rem;
$m-xl: 4rem;
$m-xl: 6rem;
Since it seems like the rest of the variables are never used and changing the naming of the variables removes the need to have every 0.5 step measure defined. @ErriGarcia thoughts?
Hello!
We have tried to fix the issue #934 as part of an Open Source Event in Madrid. We have incorporated all repeated measurements in variables. The variables were created according to this table:
Declaration of common measurements for margin and padding
$m-0: 0rem
$m-1: 0.5rem
$m-2: 1rem
$m-3: 1.5rem
$m-4: 2rem
$m-5: 2.5rem
$m-6: 3rem
$m-7: 3.5rem
$m-8: 4rem
$m-9: 4.5rem
$m-10: 5rem
$m-11: 5.5rem
$m-12: 6rem
(Padding is obviously the same but substituting m for p.)
We hope to hear from you soon and collaborate more with you in the future.