Clearly distinguish between CSS and Emotion CSS

[ob6160]

What is the purpose of this change?

We'd like to clearly indicate when Emotion CSS is exported from source-foundations.

This is so we avoid consumers rendering these exports in the context of standard CSS because they are unaware that it is Emotion CSS.

What does this change?

We change the type of the exported styles to SerializedStyles, so we can make a clear distinction between them and plain CSS, which is exported as a string.

• The input reset now returns a SerializedStyles Emotion object instead of a string
• Typography functions: headline, textSans, body, and titlepiece now return a SerializedStyles Emotion object instead of a string.
• String alternatives to these functions are provided, but please note that link underline hover styles won't work as expected unless added separately, because they depend on the parent selector which is only available in Emotion.

This change is listed as a major release because it could potentially break uses of these style blocks as they are now Emotion objects.

[changeset-bot]

🦋 Changeset detected

Latest commit: 504e2db

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 5 packages

Name	Type
@guardian/source-foundations	Major
@guardian/eslint-plugin-source-foundations	Major
@guardian/source-react-components-development-kitchen	Major
@guardian/source-react-components	Major
@guardian/eslint-plugin-source-react-components	Major

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[joecowton1]

Looks good to me - I'm not across the finer detail of all this but I trust that was discussed in the previous larger version of this PR?

[ob6160]

Looks good to me - I'm not across the finer detail of all this but I trust that was discussed in the previous larger version of this PR?

This part wasn't fully discussed. The motivation behind this was to find a way to allow projects that don't use Emotion (like parts of the AMP renderer) to still use the typography API with this set of alternative methods that just return a string.

The idea came out of a discussion with @sndrs a week or so ago, but we didn't fully decide on a solution, so this is a proposal based on our chat

[sndrs]

i'm not 100% comfortable with naming things after their type – i think it can more instructive to name things after their use, but I don't know what i'd make of body, bodyObjectStyles, bodySizes and bodyCSS etc if i was trying to get some css for styling body text either (or maybe more pertinently, if i was going to use it with emotion, which is probably also a more common use-case).

i don't want to open a larger can of worms here, but an api like fonts.body.sizes, fonts.body.css, fonts.body.emotionStyles seems more explicit to me, but comes with a greater set of changes as well as being untreeshakable :(

given we already have bodyObjectStyles, what do people think about a convention approaching:

• bodyEmotionStyles
• bodyObjectStyles
• bodySizes
• __partialFunctionalityCSSBody <- this is trying to achieve something similar to __dangerouslySetInnerHTML

cc @ob6160 @joecowton1 @tjsilver @SiAdcock

[joecowton1]

I like the ___, i think that does a good job of implying that this is a non-standard approach

[sndrs]

something that @SiAdcock pointed out in a chat just now, is that this makes emotion a peerdep of source-foundations, which feels instinctively wrong to me.

i'm not sure how we can best address this, but i guess ideally we'd move any code that needs emotion to react-components, which is obvs a bigger change.

but i can't get passed the sense that making emotion a dependency of foundations is the wrong move either...

what do other people think?

[ob6160]

something that @SiAdcock pointed out in a chat just now, is that this makes emotion a peerdep of source-foundations, which feels instinctively wrong to me.

i'm not sure how we can best address this, but i guess ideally we'd move any code that needs emotion to react-components, which is obvs a bigger change.

but i can't get passed the sense that making emotion a dependency of foundations is the wrong move either...

what do other people think?

Good spot!

I feel like if we export code that requires Emotion to run properly already, adding it as a peer-dep is reasonable.. if anything it makes that contract even clearer..

That said I think it feels wrong to me too, but maybe it feels wrong because source-foundations was never intended to have a dependency on this package.

But I think the reason why I'm veering on the side of adding it as a peerdep (for now) is because the reality of the codebase (as much as we want it not to be the case) is that we do export functionality that needs it.

[SiAdcock]

I'm not sure if I have a vote, but my instinct would be to keep Emotion out of source-foundations if possible. I know it has crept in via resets and typography, but I feel that this is a mistake that we should seek to eliminate, not something we should embrace by making Emotion a peerDependency. A mistake of my own making, I'm happy to admit 🙂

Elimination of Emotion will be easier now that it isn't pervasive. If left unchecked, in 6 months or a year from now, it would probably be a larger task. I acknowledge that this is a rather dogmatic opinion, but there can be wisdom in dogma!

A solution I would put forward, which might already have been discussed. Push any unavoidable Emotion dependency up to source-react-components, where it is already expected. Don't be afraid to make breaking changes that remove Emotion-specific CSS strings from source-foundations.

For instance, does resets.input need to exist in its current form? As noted probably not, it's used in 2 places. Feel free to improve this, remove it. It's a mistake that this is expressed in Emotion syntax.

The text-decoration-width issue is admittedly trickier. We have discussed pushing it up to the Link component, which introduces new problems. It would be worth bringing those into the open. There might be better solutions. If the problems are not resolvable technically without resorting to parent selectors, it might be worth pushing back to design to see if a compromise can be found.

This all said, there's something I am very keen to clarify: a lot of Source is mysterious. There isn't much architectural documentation (yet!). There are numerous decisions that we made at one time or another, which may have been correct at the time, or maybe not. But if there's no obvious reason for the things to continue as they are, please feel empowered to change things 🙂

[ob6160]

After some further discussion, we've decided to pause work on this and discuss our approach towards removing the dependency on emotion that we have in source-foundations instead of potentially setting it in stone by adding it as a peer dependency.

[sndrs]

I'm not sure if I have a vote

everyone has a vote!