Skip to content
This repository has been archived by the owner on Sep 1, 2021. It is now read-only.

Upgrade @guardian/types to use Theme #852

Merged
merged 8 commits into from
Oct 22, 2020
Merged

Upgrade @guardian/types to use Theme #852

merged 8 commits into from
Oct 22, 2020

Conversation

webb04
Copy link
Contributor

@webb04 webb04 commented Oct 20, 2020

Why are you doing this?

  • Updates @guardian/types to 0.5.2
  • Rename pillar helper functions to theme
  • Removed featureHeadline and soft colours that aren't being used
  • This should let us start work on special reports

@webb04 webb04 changed the title Live 1005/upgrade types Upgrade @guardian/types to use Theme Oct 20, 2020
@github-actions
Copy link

github-actions bot commented Oct 20, 2020

Before After

Copy link
Contributor

@frankie297 frankie297 left a comment

Choose a reason for hiding this comment

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

Do we have somewhere where we can add a definition of what is the difference between Theme and Pillar?

@webb04
Copy link
Contributor Author

webb04 commented Oct 20, 2020

Do we have somewhere where we can add a definition of what is the difference between Theme and Pillar?

Not sure if we have any documentation around this @JamieB-gu ?

The types repo defines it here:
https://github.com/guardian/types/blob/main/Format.ts#L3-L15

We have the problem where the special report colours need to override pillar colours on special report articles.

It might be confusing to call them a pillar as we don't treat them as pillars in some places like CAPI and the navigation bar on dotcom for example. So we updated Format to use a Theme which can be a Pillar or something Special

Copy link
Contributor

@JamieB-gu JamieB-gu left a comment

Choose a reason for hiding this comment

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

Herculean refactor here, 💪

publicationDate: Option<Date>;
className: SerializedStyles;
item: Item;
}

const Byline: FC<Props> = ({ pillar, publicationDate, className, item }) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting that this didn't previously throw an unused argument warning 🤔

src/pillarStyles.ts Outdated Show resolved Hide resolved
@JamieB-gu
Copy link
Contributor

Do we have somewhere where we can add a definition of what is the difference between Theme and Pillar?

Not sure if we have any documentation around this

It's a good point. We should probably put some markdown docs into the @guardian/types repo.

@JamieB-gu JamieB-gu mentioned this pull request Oct 20, 2020
@JamieB-gu
Copy link
Contributor

@webb04 you may find the conversation here useful for future work on this.

@webb04 webb04 merged commit 21546fd into main Oct 22, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants