Refactor Right Ad label styles #13111
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Hello 👋! When you're ready to run Chromatic, please apply the You will need to reapply the label each time you want to run Chromatic. |
|
Size Change: -16 B (0%) Total Size: 870 kB ℹ️ View Unchanged
|
emma-imber
force-pushed
the
ei/fix-dark-ad-labels
branch
from
663dab0 to
209872f
Compare
cemms1
reviewed
Jan 8, 2025
Comment on lines
94
to
95
| border-top-color: ${palette.neutral[20]}; | ||
| color: ${palette.neutral[86]}; |
There was a problem hiding this comment.
maybe we should update these too?
There was a problem hiding this comment.
Yep agreed - I think we can actually handle this more robustly by using schemedPalette and adding some palette declarations to handle the edge case of audio/video articles? I've had a go - let me know what you think
cemms1
reviewed
Jan 8, 2025
| @@ -90,7 +90,7 @@ const outOfPageStyles = css` | |||
|
|
|||
| const darkLabelStyles = css` | |||
| .ad-slot[data-label-show='true']:not(.ad-slot--interscroller)::before { | |||
| background-color: transparent; | |||
| background-color: ${schemedPalette('--article-inner-background')}; | |||
There was a problem hiding this comment.
Should we use this, as this is used elsewhere in the file
Suggested change
| background-color: ${schemedPalette('--article-inner-background')}; | |
| background-color: ${schemedPalette('--ad-background-article-inner')}; |
OR
Suggested change
| background-color: ${schemedPalette('--article-inner-background')}; | |
| background-color: ${schemedPalette('--ad-background')}; |
There was a problem hiding this comment.
Ah I don't think the --ad-background-article-inner declaration actually handles the case of Audio/Video articles yet! So in light mode on these articles it wouldn't return the right value. I can update the declaration to handle Audio/Video articles and then use that value
cemms1
approved these changes
Jan 8, 2025
emma-imber
changed the title
cemms1
approved these changes
Jan 9, 2025
|
Seen on PROD (merged by @emma-imber 9 minutes and 18 seconds ago) Please check your changes! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What does this change?
Refactors the Right ad slot label styles to handle different styling on Audio/Video articles, and on Labs Video/Audio articles. Using palette declarations to handle Video/Audio articles also means that we don't need to add a prop in the
MostViewedRightWithAdcomponent.Why?
At the moment ad labels on dark backgrounds (eg video or audio pages) look broken, and are not readable. A refactor of how these styles works is a more robust fix for the issue than updating the
background-colorvalue. All other styles have been kept the same.Screenshots
Video article
Labs video article
Standard labs article
Standard article