LIVE-1581: Item Extras Context (RFC)

[faresite]

Item Extras Context

We have discussed using a React Context to provide the Item. This PR presents creating the Item context and using it in the Header components. The PR is a proposal and doesn't prove the usefulness of it yet as that would probably need wider implementation (removing all item props) and potentially adding derived state, for example ItemExtras extends Item to add computed properties like kickerColor, themeStyles or even fullWidthHeader.

Update: I've added some derived state.

Context Wrapper

The context wrapper's purpose here is to make it easy to create a type safe context without passing a default value at time of instantiation. Anywhere you need to use useItem you're guaranteed it would:
A. return an Item
B. throw an Error if you try to use it outside of the context Provider.

NB. Storybook breaks but until we're happy to proceed using context there's no point of fixing those.

[github-actions]

Standard

Before	After

Immersive

Before	After

Feature

Before	After

Gallery

Before	After

Opinion

Before	After

Review

Before	After

Analysis

Before	After

EditionsStandard

Before	After

EditionsOpinion

Before	After

EditionsReview

Before	After

[joecowton1]

Nice work on the ContextWrapper 👌

I think as you said in your own comment, this doesn't really demonstrate a potential use-case for Context in Editions as it is - do you think its worth extending this PR in the way's you've suggested (kickerColor / format etc) so we can properly evaluate what the benefits would be?

[JamieB-gu]

Doing something like this was discussed on DCR a while back: guardian/dotcom-rendering#801

Ultimately they decided not to take this approach - perhaps @oliverlloyd could elaborate on the reason why?

@nicl suggested some limitations of this approach on that original PR which I tend to agree with (3 probably doesn't apply to Editions, but I think 1, 2 and 4 do).

[oliverlloyd]

I wrote that original PR looking to add Context to DCR for similar reasons. It's worth pointing out that, a year later, I'm actually glad I lost that argument!

I still spend a large part of my time prop drilling and this remains the same mind numbingly annoying task it always was, but I really like how it feels like DCR components are boringly simple, even as we grow and scale the codebase. A DCR component has props and.... that's it! Testing is easy, refactoring is easy (albeit boring because of all the prop drilling) and it's simple to follow the logic of a request for debugging.

But I'm still waiting for that moment when the scales tip and context starts to make sense. Perhaps we already passed it with the addition of the prop drilling beasts, format and palette? Maybe. It's hard to evaluate or measure these things.

There's no right or wrong, Context can magically simplify things in a really nice way. But, like all magical potions, it also has a price so drink it with care.

[oliverlloyd]

Airy talk of magic aside, one concrete reason for *-rendering projects to be wary of Context is it will make hoisting components into shared repos later on much harder. The more components work in isolation and the less they depend on globals, the easier it is to compose them out of context (pun intended).

[faresite]

@oliverlloyd Thank you very much for your input!

I hear your point and the points made in that PR and I'm on board with a lot of it. Just to play the devil's advocate here:

• Re magic, very personal opinion, but we're bought into React if we like it or not, and truth is Context doesn't seem that magical anymore especially if you're using it through hooks which is basically a big chunk what React is about these days.
• Fully agree that it's easier to test, luckily there are ways around it that shouldn't be too hard with our use case here as we're not dealing with any side effects.
• One difference I can see with our use of the context is the aspiration to derive state in one place. If it was just prop drilling I don't think I would feel a strong need to propose this even. We have to compute basic things like kickerColor or getFormat etc.. out of the prop in so many components 😅 This seems to me is getting quite messy.. but understand if it's just my own personal experience.
• Re shared repo, I might be a bit pessimistic but we already diverge our components between editions-rendering and apps-rendering .. I wonder how feasible it will be to compose DCR / AR / ER components in the future even if we wanted to. If we really want to head this way, I feel like we've accumulated some debt and we should do something about it sooner than later, unless I misunderstood you?

Having said this, I can see the other side as well, but before I close this PR, I wanted to share some of the thoughts first and see what you all think.

[oliverlloyd]

One difference I can see with our use of the context is the aspiration to derive state in one place

👍 I think this approach where you're taking your project goals and considering these when making decisions makes a lot of sense. I don't know what these are so don't trust me! So much of these types of discussions can be subjetive and so it's important to try and identify real world use cases and use these to make the call.

shared repo

Honestly, I'm also unsure about this. I think that the process of building components as though they might be shared is a great thing to do because you build better componets because of it. But actually sharing them is scary because that coupling suddenly makes it harder to change them. So far the components that we have hoisted have been the static ones we don't expect to change and this has gone well and I think there's still room to add more, but maybe it's nice to go about this process slowly. We should be playing the long game here, if we get the shared components right now we can benefit from that for years and years to come but if we rush it to meet short term goals we might end up rebuilding it all again in 3 years 😐

By the way, thanks so much for your ignite talk earlier - that was really something.

[faresite]

because you build better components because of it

This is possibly my favourite argument of all! Agree 💯

So far the components that we have hoisted have been the static ones we don't expect to change and this has gone well

I'd love to see how this is done, are they organised separately from other components? Do you mind linking to some of these @oliverlloyd so I can understand better?

[oliverlloyd]

I'd love to see how this is done, are they organised separately from other components?

We have the following shared components sitting in Source:

https://theguardian.design/2a1e5182b/p/77ee17-core-components

Some things are obvious and used a lot. Button is classic and things like icons. But some are more Guardian orientated, like Lines.

Not sure why Lines isn't in the design webpage linked above, but the code is here

We've been discussing promoting other components to the same location, things like Header, Footer, Logo, EditionDropdown etc., but it's not actively happening at the moment. I think before we really dive into that we're working on sharing more conceptual things like types (in particular, Format) and things like https://github.com/guardian/image-rendering/. But you probably already know about all that.

[joecowton1]

@faresite thanks for expanding the scope of the PR :)

Even with the expanded example, I'm still not convinced it provides is with much beyond removing a small amount of prop-drilling.

I think the thing (as I see it) which makes these templates complex is the nature of the designs - and the various inconsistencies they contain as we've discussed a lot recently with Ben. I share your desire to simplify our approach but I don't think this is going to help in that struggle particularly.

The Format system does a good job of codifying a lot of the variations, if Ben can remove a decent amount of the inconsistencies I think then our codebase will make a lot more sense. That should do more of a job of tidying up this project than I think we can achieve using Context.

TL;DR complexity within Editions templates is caused by contradictions in the designs, not excessive prop-drilling.

[faresite]