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.

Sign up for GitHub

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

Jump to bottom

[AR] Migrate from @guardian/types to @guardian/libs #3376

Closed
wants to merge 3 commits into from
Closed

[AR] Migrate from @guardian/types to @guardian/libs #3376

wants to merge 3 commits into from

Conversation

jamie-lynch
Copy link
Contributor

@jamie-lynch jamie-lynch commented Sep 6, 2021 •
edited
Loading

What does this change?

This PR migrates from the @guardian/types library to @guardian/libs as @guardian/types is now deprecated.

How to test

Run all validation and ensure that everything still runs as expected.

TODO

@jamie-lynch jamie-lynch mentioned this pull request Sep 9, 2021
Closed
@jamie-lynch
Copy link
Contributor Author

As well as the dependencies that need bumping, there's also an issue in src/renderer.ts where calls to React.createElement (aliased as h) are expecting the old types (e.g. Format instead of ArticleFormat). I've tried to figure out how this works but I can't unpick how createElement knows about Guardian types. Can somebody take a look please (happy to pair on it)?

@jamie-lynch jamie-lynch marked this pull request as ready for review September 27, 2021 15:24
@jamie-lynch
Copy link
Contributor Author

Update the version of @guardian/image-rendering

As far as I could tell, @guardian/image-rendering was being installed from GitHub, targeting this commit (from this PR). I've updated it to point at the latest commit on main instead but not sure if that's how this should work..?

jamie-lynch
jamie-lynch commented Oct 4, 2021
View reviewed changes
apps-rendering/src/components/interactiveAtom.tsx
Comment on lines +34 to +40
${pageFonts}

@media (prefers-color-scheme: dark) {
body {
background: white;
padding: ${remSpace[3]} !important;
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is obviously not types -> libs related. I think it must have happened when I ran npm run lint -- --fix

apps-rendering/src/server/csp.ts
@@ -98,7 +99,7 @@ const buildCsp = (
? 'https://platform.twitter.com https://syndication.twitter.com https://pbs.twimg.com data:'
: ''
};
script-src 'self' ${assetHashes(scripts)}
script-src 'self' ${assetHashes(scripts)}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is another format change, probably from npm run lint -- --fix

@SiAdcock SiAdcock mentioned this pull request Oct 13, 2021
Merged
@jamie-lynch
Copy link
Contributor Author

Closing this as its super-seeded by #3621 🎉 , which was probably significantly less painful that fixing all of these merge conflicts 😨

@mxdvl mxdvl deleted the ar/types-to-libs branch February 25, 2022 14:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JamieB-gu JamieB-gu Awaiting requested review from JamieB-gu

@joecowton1 joecowton1 Awaiting requested review from joecowton1

@abeddow91 abeddow91 Awaiting requested review from abeddow91

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
1 participant
@jamie-lynch