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

Add SpecialReportAlt theme article format #132

Merged
merged 1 commit into from
Oct 11, 2022

Conversation

DanielCliftonGuardian
Copy link
Contributor

@DanielCliftonGuardian DanielCliftonGuardian commented Oct 10, 2022

Co-Authored-By: Ioanna Kokkini [email protected]

What are you changing?

This adds SpecialReportAlt theme in ArticleSpecial.

BREAKING CHANGE: Consumers of ArticleFormat and/or ArticleTheme will need to update to handle this change. This may involve updating component props, fixtures, switch and if statements, etc.

Why?

We need to support a new article type of SpecialReportAlt.

@changeset-bot
Copy link

changeset-bot bot commented Oct 10, 2022

🦋 Changeset detected

Latest commit: 3a2c68e

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

This PR includes changesets to release 2 packages
Name Type
@guardian/libs Major
@csnx/npm-package Patch

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

@github-actions github-actions bot added @guardian/libs 📦 npm Affects a @guardian package on NPM labels Oct 10, 2022
@DanielCliftonGuardian DanielCliftonGuardian marked this pull request as ready for review October 11, 2022 11:02
@DanielCliftonGuardian DanielCliftonGuardian requested a review from a team October 11, 2022 11:02
@DanielCliftonGuardian DanielCliftonGuardian requested review from a team as code owners October 11, 2022 11:02
@joecowton1
Copy link
Contributor

Left a comment to this effect in the code, but i'm interested in whether this actually is a breaking change: if we're just adding a new type of theme will that break anything? My gut feeling is that consumers should be fine to continue as they were without it effecting anything.

@ioannakok ioannakok requested a review from JamieB-gu October 11, 2022 11:13
@ioannakok
Copy link
Contributor

Thanks for your comment @joecowton1! From a quick look in the DCR codebase, it seems there are places where we switch over format.theme without having a default case, e.g.
https://github.com/guardian/dotcom-rendering/blob/main/dotcom-rendering/src/web/lib/decidePalette.ts#L202-L217

At the moment TS is not complaining but we have a PR to fix this: guardian/dotcom-rendering#6105 hence why we think this is a breaking change.

@joecowton1 joecowton1 self-requested a review October 11, 2022 12:03
@DanielCliftonGuardian DanielCliftonGuardian merged commit e0dea0b into main Oct 11, 2022
@DanielCliftonGuardian DanielCliftonGuardian deleted the Add-special-report-alt branch October 11, 2022 13:18
@oliverlloyd
Copy link
Contributor

It's true that this change could cause some consumers to break, so it is breaking in this (important!) sense. Arguably though, we should probably look to update this code to be more resilient and allow these sorts of changes to go out as features, which perhaps, semantically, they are?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@guardian/libs 📦 npm Affects a @guardian package on NPM
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants