Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Automat 0.3 changes #194

Merged
merged 3 commits into from
Jul 28, 2020
Merged

Automat 0.3 changes #194

merged 3 commits into from
Jul 28, 2020

Conversation

nicl
Copy link
Contributor

@nicl nicl commented Jul 22, 2020

DO NOT MERGE - required changes in platforms first. guardian/dotcom-rendering#1758 and also the Frontend PR which must be merged at the same time (so we'll have a few minutes with no subs/contributions banners on Frontend).

What does this change?

Various changes to support the newer automat clients. See:

https://docs.google.com/document/d/1CVkWvCo5SlJjedPmv0Uh6gQ6mwFsGydE2RvZzo6uxGk/edit?userstoinvite=tom.forbes%40guardian.co.uk&ts=5f16bd85&actionButton=1#

https://github.com/guardian/automat-client-v2/

for more info here.

The summary is:

  • switch from emotion to @emotion/core
  • add @guardian/automat-deps to help ensure react/emotion versions safe
  • apply reduced globals list (automat no longer provides emotion and emotion-theming)

How to test

Run the tests, and also test with Frontend or DCR.

How can we measure success?

Able to support modules on Frontend (as a separate change but this is a step towards that).

Have we considered potential risks?

I haven't tested the banners yet.

@nicl nicl force-pushed the nicl/automat-0.3-changes branch 2 times, most recently from e18ad2f to eb02758 Compare July 22, 2020 16:55
@nicl nicl changed the title Nicl/automat 0.3 changes Automat 0.3 changes Jul 22, 2020
Copy link
Contributor

@tompretty tompretty left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this may have broken the components in storybook for now. It looks like the css prop isn't being handled correctly by storybook?
Screenshot 2020-07-23 at 08 25 54

@nicl
Copy link
Contributor Author

nicl commented Jul 23, 2020

@tompretty ah thanks - yes almost certainly. I probably need to add the babel plugin - will take a look.

Update: this should be okay now.

@nicl nicl force-pushed the nicl/automat-0.3-changes branch 2 times, most recently from 6736e20 to 4057d5c Compare July 23, 2020 14:19
nicl added a commit to guardian/frontend that referenced this pull request Jul 23, 2020
Copy link
Contributor

@tompretty tompretty left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great - I very excited for this one! 🚀

Copy link
Member

@tjmw tjmw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💯 Looks great!

nicl added a commit to guardian/frontend that referenced this pull request Jul 27, 2020
nicl added 3 commits July 27, 2020 13:18
This is required to use the emotion cache, which Automat uses as
part of its shadow dom support.

In addition, Source uses @emotion/core and standardising helps
avoid having to load both.

Note, an interesting discussion on the differences between
emotion and @emotion/core can be found here:
emotion-js/emotion#1883.
Currently failing on .? and ?? operators.
@nicl nicl force-pushed the nicl/automat-0.3-changes branch from 4057d5c to 64c3e37 Compare July 27, 2020 12:20
nicl added a commit to guardian/frontend that referenced this pull request Jul 28, 2020
@nicl nicl merged commit 633b951 into master Jul 28, 2020
@tompretty tompretty deleted the nicl/automat-0.3-changes branch March 3, 2021 09:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants