-
Notifications
You must be signed in to change notification settings - Fork 153
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
Youtube Regrets 2022 - Recommendations section #9254 #9324
Conversation
I've added @tbrlpld to this PR as he's the one doing reviews from the Mozilla team |
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.
Just optional minor nitpicks.
...agtailpages/templates/wagtailpages/pages/youtube-regrets-2022/fragments/recommendations.html
Outdated
Show resolved
Hide resolved
...agtailpages/templates/wagtailpages/pages/youtube-regrets-2022/fragments/recommendations.html
Outdated
Show resolved
Hide resolved
...agtailpages/templates/wagtailpages/pages/youtube-regrets-2022/fragments/recommendations.html
Outdated
Show resolved
Hide resolved
...agtailpages/templates/wagtailpages/pages/youtube-regrets-2022/fragments/recommendations.html
Outdated
Show resolved
Hide resolved
…es/youtube-regrets-2022/fragments/recommendations.html Fix whitespace. Co-authored-by: Tibor Leupold <[email protected]>
…es/youtube-regrets-2022/fragments/recommendations.html Fix whitespace. Co-authored-by: Tibor Leupold <[email protected]>
…es/youtube-regrets-2022/fragments/recommendations.html Fix whitespace. Co-authored-by: Tibor Leupold <[email protected]>
…es/youtube-regrets-2022/fragments/recommendations.html Fix whitespace. Co-authored-by: Tibor Leupold <[email protected]>
@siimonevans This has conflicts |
…dations-section # Conflicts: # network-api/networkapi/wagtailpages/templates/wagtailpages/pages/youtube-regrets-2022/youtube_regrets_2022.html
Youtube Regrets 2022 - Hero carousel section #9260
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.
Looks good to me, just added a few suggestions to eliminate the mathsss
|
||
@media screen and (min-width: $bp-md) { | ||
font-size: 52px; | ||
line-height: calc(60 / 52); |
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 equals 1.15 too so we don't need it here haha
line-height: calc(60 / 52); |
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.
If we do this, can we use values from the design system?
|
||
&__title { | ||
font-size: 30px; | ||
line-height: calc(34 / 30); |
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.
My sole purpose in life is to eliminate math 🤖
line-height: calc(34 / 30); | |
line-height: 1.15; |
|
||
&__recommendation { | ||
font-size: 24px; | ||
line-height: calc(30 / 24); |
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.
line-height: calc(30 / 24); | |
line-height: 1.25; |
|
||
@media screen and (min-width: $bp-md) { | ||
font-size: 28px; | ||
line-height: calc(30 / 28); |
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.
Even though we are reusing a bunch it would be nice to remove this needless math haha
line-height: calc(30 / 28); | |
line-height: 1; |
Ups. This is already merged. |
@tbrlpld hahah oops! No big deal anyway :) |
Addresses #9254
This PR adds a recommendations section (in line with the existing 2021 component with updated copy).
Screenshots