Introduce decidePalette to DCR
#2378
Conversation
nitro-marky
requested review from
gtrufitt,
jlieb10,
liywjl,
oliverlloyd and
shtukas
as code owners
|
Size Change: +510 B (0%) Total Size: 824 kB
ℹ️ View Unchanged
|
|
This guardian/types#61, along with this #2365 will unblock this PR |
|
We need to replace |
There was a problem hiding this comment.
This is exciting!
It got me curious about a couple of design decisions that you've made: prop-drilling and the API. I've rambled a bit below, sketching out my understanding of how you've gone about these and asking a couple questions. Let me know if I've understood correctly and what you think!
Prop-drilling
By this I mean passing the palette down from the "top-level" (i.e. DecideLayout). As far as I can tell this is one of three options, the other two being:
- Pass
Formatdown and derive subsets of the palette at the component level, which is what AR has tentatively started doing. - Use a Theme to dependency inject the palette without prop-drilling, which I believe was the direction @oliverlloyd was originally leaning in.
What made you settle on prop-drilling in the end?
The API
The palette here is a single large object, derived from Format, with the colours pulled from nested fields. We discussed the fact that DCR would probably go this route while AR used individual functions, but I can't remember why 🤔.
I'm also interested in the naming system and how that corresponds to what Source does. There's a discussion about this here (featuring @SiAdcock), with the hierarchy defined by Source as:
[theme][property].[component][variant][state][mode]
For a headline text colour I believe this maps to:
property:textcomponent:headlinevariant:primarymode:inverse(only used for dark mode)
and this is why the AR version of this looks something like:
{
text: {
headlinePrimary: text.headlinePrimary(format),
headlinePrimaryInverse: text.headlinePrimaryInverse(format),
},
background: {
headlinePrimary: background.headlinePrimary(format),
headlinePrimaryInverse: background.headlinePrimaryInverse(format),
},
}(I've included background as a parallel example).
I notice that the API defined here looks like this: palette.headline.colour. I think colour corresponds to property and therefore this flips the order of property and component from what I outlined above? Is that right?
We've looked at doing less prop drilling before but we always end up keeping it. I don't think there's anything wrong with using a I think this is definately are area where there's a case to be made for it but I think we want to be cautious and approach it in a new PR at a later stage when we better understand our use case and have a known api. I'm also wondering if using a It's perhaps worth pointing out that I'm - so far - grateful that my previous attempts to add Context failed and I'm actually enjoying good old prop drilling (not all the file changes, that bit is less fun, but the simplicity of it) |
This was, I think, related to how we prefer prop drilling in DCR. It certainly feels more natural to me to take this appraoch and I'm also a fan of the discoverability of being able to autocomplete on |
This is up for grabs. I definately think it makes sense to align on a pattern and it sounds like you've got a well established one already. I not sure though about having function names the same as property names. I worry this would make refactoring difficult (hard to search or do find and replaces). Maybe instead of headlinePrimary: text.headlinePrimary(format),you could headlinePrimary: text.getHeadlinePrimary(format),The |
Is there a need for |
Would this pattern mean the calling component has to decide between dark and light mode? What about having this logic inside text: {
headlinePrimary: dark ? text.headlinePrimary(format) : text.headlinePrimaryInverse(format),
},(or similar, that api could be better!) |
The palette object would be one big object and the getter functions would handle the light and dark I think? Just my input reading through:
I think we should try and create a type for the palette in @guardian/types and cascade that into AR and DCR - it may be that there are a number of properties on the type that DCR doesn't use in relation particularly to dark mode (or as Oliver mentions the 'secondary' headline type), but having a split in naming or even where the palette is used between the platforms will make it difficult to maintain. |
I think we definately want this 👍 But maybe not straight away. The chore of having to make changes to two repos could be a drag on development when initially creating this object. Or we use a monorepo 😄 |
Fair, I think that's a reasonable preference. What are your thoughts about passing down the whole
By this do you mean that in the
Good question, I don't know. Perhaps one for @SiAdcock?
It does, but typically it already does that because there's often more than one property to change for dark mode. Regarding having it inside
Agree with both of these statements. We started incubating it in AR as well but haven't progressed too far because of other priorities. |
I think for DCR, it makes sense to pass down the whole I'm very much of the opinion that we should keep an open mind here though and this could well be an area we come back to later after we've spent some time doing things one way and learnt more about how that felt
I think I just mean that the One problem with this could be if I am using find and replace or Another is, like this then we're constrained to always have one function per property, you can't have one generic function used in multiple places. Is that maybe intentional though?
Yeah, I think you're right, CSS would be a good solution here. But lets take this one out of this PR and discuss it seperately |
|
@JamieB-gu after discussing the api (same name business) with @nitro-marky we agreed that the approach used in AR is actually fine and so I've modified the code to align on this I think for drilling And let's talk dark mode, but not here. |
I think in this case it probably is because you'll want to keep them in sync?
Maybe? In this case we're defining two APIs that do the same thing:
We probably want every property to have an equivalent function, and to have the property and function provide the same values and be kept in sync? I think the important thing to remember is that the function versions are also part of the public API - they're just being dog-fooded under the hood by the Again, if you accidentally use the wrong one (e.g. try to use a function where you're supposed to be using a property) the compiler should tell you.
👍 |
What does this change?
Work with @oliverlloyd to begin the implementation of a
decidePalettefunction that, givenformat: Format, will return the correct colours to be used throughout DCR simply by calling the functiondecidePallet(format)whereformatequals:See: https://github.com/guardian/types/blob/main/src/format.ts
We also add the definition of
formattodecideLayout. This is where we build theformatobject for an article, based on the CAPI response.Any thoughts would be greatly appreciated.