Skip to content

Conversation

@sarayourfriend
Copy link
Contributor

@sarayourfriend sarayourfriend commented Oct 19, 2020

Description

Use CSS-in-JS for RadioControl.

Note: I discovered a bug documented here while working on this: #26293. This is a pre-existing bug that I'm not sure exactly how to fix. I think it should be done in the context of a separate pull request once this one is merged and once there is an answer about the intention for the radio control in the bug issue.

How has this been tested?

Verified there are no differences between this branch and production in Storybook. Also checked in Gutenberg itself.

Captura de Tela 2020-10-27 às 08 22 16

Types of changes

Non-breaking CSS-in-JS refactor. All classnames are preserved allowing folks to continue targeting the classes that were already there.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.

@sarayourfriend sarayourfriend changed the title Use CSS-in-JS for RadioControl RadioControl: Use CSS-in-JS Oct 19, 2020
@sarayourfriend sarayourfriend marked this pull request as draft October 19, 2020 18:50
@sarayourfriend sarayourfriend marked this pull request as ready for review October 19, 2020 18:52
@sarayourfriend sarayourfriend self-assigned this Oct 20, 2020
Copy link

@ItsJonQ ItsJonQ left a comment

Choose a reason for hiding this comment

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

@saramarcondes Haiii! I left a suggestion for verbose box-shadow vs Prettier 😂

@ItsJonQ
Copy link

ItsJonQ commented Oct 28, 2020

@saramarcondes We're starting to venture into components that require references to the var( --wp-admin-theme-color ) variable. We currently don't have an automated way to handle this yet, which means it won't work currently in IE11.

I left some more thoughts here:
#26340 (comment)

I don't think there's a good solution yet, other than to manually hardcode the fallbacks 🙈

We could create a "mixin" that can output the property for use.. something like:

const StyledInput = styled.input`
  ${variable('background', 'wp-admin-theme-color', 'blue')}
`

That's kind of how you'd do it in Sass. I really don't like this solution. It's quite limited (e.g. handling multiple values) and feels... too abstract. But it is an option if we don't want to hand-write fallbacks.

Would love your thoughts!

@sarayourfriend
Copy link
Contributor Author

sarayourfriend commented Oct 28, 2020

@ItsJonQ Shall we compromise and update ui.theme in the colors values to be the following:

css`
  color: blue;
  color: var( --... );
`;

I think that might be a better option than trying to come up with a new solution for this when hopefully we could find an already baked one?

@ItsJonQ
Copy link

ItsJonQ commented Oct 28, 2020

@saramarcondes We can certainly give that a shot 🎉

@sarayourfriend
Copy link
Contributor Author

I'll update this in the FormToggle PR as it's older and I'm hoping to get it merged sooner 🙂

@gziolo gziolo added the [Package] Components /packages/components label Nov 8, 2020
@sarayourfriend
Copy link
Contributor Author

Closing in favor of focusing on G2-components integration.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[Package] Components /packages/components

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants