-
-
Notifications
You must be signed in to change notification settings - Fork 99
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
Backport Sass from upstream #443
Changes from 3 commits
d625036
5c75f8a
3a0283b
5febd28
d1ac697
141dc1c
5876bd3
f7c5a91
676899c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
XhmikosR marked this conversation as resolved.
Show resolved
Hide resolved
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. FYI, some classes defined in this document are not used in the blog, such as There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, but I'd rather keep the used files as closed as possible. Alternatively, we can comment out the unused crap and add a comment There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Got no problem with that, just wanted to spot it in case you weren't aware of it. |
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.
Maybe the surrounding HTML code is different in the blog, but
margin: 2rem 0;
causes a regression making the ad not horizontally centered at some breakpoints:The easiest fix would be to do
margin: 2rem auto 0;
instead.If we want exactly the same Scss file, we should rather dig into the HTML structure surrounding the ad "component".
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.
Feel free to push your changes as always
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.
What's your preferred approach? Having exactly the same Sass file for an easier maintenance?
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 took time to dig into this topic in detail and pushed 5febd28 to fix the issue.
We've gathered upstream
_ad.scss
as is. However, upstream, there's an extra rule for#carbonads
insite/assets/scss/_masthead.scss
:Ads are displayed in different contexts upstream so that an extra rule is not always needed. For the blog, it must be used. So I've added the extra
auto
directive and added a comment to try to show that this is specific.You can remove or reword this comment the way you think it's going to be the most helpful in terms of maintenance; our future selves should understand at some point the following:
_ad.scss
= upstream:_ad.scss
+ extraauto
rule.