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

Add theme prop support to Radio buttons #1147

Merged
merged 8 commits into from
Feb 6, 2024
Merged

Add theme prop support to Radio buttons #1147

merged 8 commits into from
Feb 6, 2024

Conversation

jamesmockett
Copy link
Contributor

@jamesmockett jamesmockett commented Feb 1, 2024 •
edited
Loading

What are you changing?

  • Adds support for theme prop and updated theme format, deprecating use of existing themes and ThemeProvider.
  • Theming has been added to the Radio and RadioGroup components.
  • The transformOldProviderTheme method that translates existing themes to the new format has been moved to the theme module so that it can be used by both components without having to duplicate the code.

Additional information

Brand themed Radio groups currently have an incorrectly coloured label as the Legend component has not yet been updated with theme prop support.

@changeset-bot changeset-bot
Copy link

changeset-bot bot commented Feb 1, 2024 •
edited
Loading

⚠️ No Changeset found

Latest commit: 4ebc345

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Feb 1, 2024

Tip

Once this PR is ready to go, add the run_chromatic label to run the Chromatic tests.

This saves us a lot of money by not running the tests before we need them.

@github-actions github-actions bot added the 📦 npm Affects a @guardian package on NPM label Feb 1, 2024
@jamesmockett jamesmockett changed the base branch from main to release/source-react-components-themeing February 1, 2024 17:43
@jamesmockett jamesmockett added the run_chromatic Runs chromatic when label is applied label Feb 1, 2024
@jamesmockett jamesmockett marked this pull request as ready for review February 1, 2024 18:18
@jamesmockett jamesmockett requested a review from a team as a code owner February 1, 2024 18:18
@jamesmockett jamesmockett linked an issue Feb 1, 2024 that may be closed by this pull request
Add theme prop, deprecate old theme, transform from old to new theme for component - Radio #1144
Open
@jamesmockett jamesmockett requested a review from a team as a code owner February 1, 2024 18:20
oliverabrahams
oliverabrahams approved these changes Feb 6, 2024
View reviewed changes
Copy link
Contributor

@oliverabrahams oliverabrahams left a comment

Choose a reason for hiding this comment

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

LGTM

libs/@guardian/source-react-components/src/radio/theme.ts
textSupporting: palette.brand[800],
};

export const transformOldProviderTheme = (
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice! This makes more sense to have this function here

libs/@guardian/source-react-components/src/radio/RadioGroup.tsx
Copy link
Contributor

Choose a reason for hiding this comment

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

Will have to update the Inline error and legend once their PR is merged

@jamesmockett jamesmockett merged commit a9bb529 into release/source-react-components-themeing Feb 6, 2024
15 of 16 checks passed
@jamesmockett jamesmockett deleted the jm/radio-theming branch February 6, 2024 15:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oliverabrahams oliverabrahams oliverabrahams approved these changes

Assignees
No one assigned
Labels
📦 npm Affects a @guardian package on NPM run_chromatic Runs chromatic when label is applied
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add theme prop, deprecate old theme, transform from old to new theme for component - Radio
2 participants
@jamesmockett @oliverabrahams