Darkmode – Avatar 👤 #9257
Merged
Darkmode – Avatar 👤 #9257
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
|
Size Change: -10.4 kB (-1%) Total Size: 1.41 MB
ℹ️ View Unchanged
|
mxdvl
force-pushed
the
darkmode/avatar
branch
from
748a4d9 to
d58f54a
Compare
mxdvl
force-pushed
the
darkmode/avatar
branch
2 times, most recently
from
dd13d46 to
6ad6a66
Compare
cemms1
reviewed
Oct 23, 2023
There was a problem hiding this comment.
Can see there's been a refactor to take format out of <Avatar> into a wrapper instead, though feel this makes it slightly tricky to see the changes on GitHub.
Main thing though is this naming convention stuff as I feel it's best to be consistent whatever we do - do you have strong opinions? Could take it to the team
dotcom-rendering/src/palette.ts
Outdated
Comment on lines
242
to
262
| '--background-avatar': { | ||
| light: backgroundAvatarLight, | ||
| dark: backgroundAvatarDark, | ||
| }, | ||
| '--background-article': { | ||
| light: backgroundArticleLight, | ||
| dark: backgroundArticleDark, | ||
| }, |
There was a problem hiding this comment.
the examples for headline have a different naming convention to these: --background-avatar vs --avatar-background.
I think I prefer the original style ie. --avatar-background but maybe we should confirm as a team which way we want to go. Having a mixture is a bit confusing
There was a problem hiding this comment.
I followed the pattern established in AR of splitting specific colour functions by type such as “text”, “background”, “line”, etc.
I can see that as this approach is not nested, it may be more accurate to organise things by themes such as “star”, “avatar”, “card”, etc.
Emoji react:
- 🎉 type (what we did in AR / DCR previously)
- 🚀 themes (what you’re suggesting @cemms1 & @sophie-macmillan, and is growing on me)
There was a problem hiding this comment.
Ah I hadn't really thought about how to best organise all of the colours! I was mainly pointing out the naming convention. Feel like we might need a discussion about organising the palette functions in the not-too-distant-future as it's gonna get big!
There was a problem hiding this comment.
Naming ≈ Organising 😉
This is a necessity! |
mxdvl
force-pushed
the
darkmode/avatar
branch
from
6ad6a66 to
c2ac125
Compare
mxdvl
force-pushed
the
darkmode/avatar
branch
from
c2ac125 to
7d445ee
Compare
mxdvl
force-pushed
the
darkmode/avatar
branch
from
439769c to
321b824
Compare
This was referenced Oct 24, 2023
mxdvl
force-pushed
the
darkmode/avatar
branch
from
6723632 to
4e2e075
Compare
mxdvl
force-pushed
the
mxdvl/format-boundary
branch
from
6d5b97e to
7d96ead
Compare
mxdvl
force-pushed
the
darkmode/avatar
branch
from
d5e0afb to
db63046
Compare
sophie-macmillan
requested changes
Oct 25, 2023
mxdvl
force-pushed
the
darkmode/avatar
branch
2 times, most recently
from
bb71816 to
85dad4a
Compare
mxdvl
force-pushed
the
darkmode/avatar
branch
from
85dad4a to
b6059ec
Compare
sophie-macmillan
approved these changes
Oct 27, 2023
simplified via the use of the new CSS var palette
Rich links may not share the same format as the article that they are on, so we need to override the CSS custom properties when there is an Avatar, e.g. on Opinion pieces.
currently unused
link to relevant issue
ensure that all descendant elements are styled correctly
this ensures that this component does not break existing layouts. For more information, see: https://developer.mozilla.org/en-US/docs/Web/CSS/display-box#contents Support is good, accessibility issues do not impact this `div` element: https://caniuse.com/css-display-contents
mxdvl
force-pushed
the
darkmode/avatar
branch
from
b6059ec to
1bb1e19
Compare
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?
Avatar.tsxdecidePalette’sbackground.avatarWhy?
Closes #9238
Screenshots