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

chore(xplat): Add support for nested selectors to Griffel to StyleX converter #30791

Merged

Conversation

behowell
Copy link
Contributor

ℹ️ Note this is being merged into the xplat branch

Previous Behavior

The convertGriffelToStyleX platform adapter didn't support nested selectors.

New Behavior

The converter now adapts the griffel format of nested selectors:

{
  color: 'red',
  ':hover': {
    color: 'green',
    '@media (forced-colors: active)': {
       color: 'highlighttext',
     },
  },
}

into the stylex format of nested selectors:

{
  color: {
    default: 'red',
    ':hover': {
      default: 'green',
      '@media (forced-colors: active)': 'highlighttext',
    },
  },
}

@behowell behowell self-assigned this Mar 15, 2024
@behowell behowell requested a review from a team as a code owner March 15, 2024 19:40
@github-actions github-actions bot added this to the March Project Cycle Q1 2024 milestone Mar 15, 2024
Copy link

@JasonVMo JasonVMo left a comment

Choose a reason for hiding this comment

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

Good catch on this one, I hadn't noticed the signature differences between stylex and griffel for these.

[atRule: `@${string}`]: string | number;
type ValueWithSelectors = {
default?: string | number | ValueWithSelectors | null;
[pseudoClass: `:${string}`]: string | number | ValueWithSelectors;

Choose a reason for hiding this comment

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

Interesting, didn't realize index signatures could be added this way.

} else {
throw new Error(`Unsupported nested selector: '${selector}' '${key}'`);
console.warn(`Unsupported selector `, [...selectors, key]);

Choose a reason for hiding this comment

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

If I follow the logic now, we will emit warnings with unsupported selectors but not end up passing them to stylex. I think that is better than having the hard errors.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, we'll need a longer-term solution to catch and fix these cases at development time, like a lint rule or something. But for now, this at least makes it so it doesn't crash with an invalid selector.

@behowell behowell merged commit 5139c70 into microsoft:xplat Mar 16, 2024
11 of 13 checks passed
@behowell behowell deleted the xplat-griffel-stylex-nested-selectors branch March 16, 2024 07:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants