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

feat: Allow stencil modifiers-in-selectors #2741

Conversation

NicholasBoll
Copy link
Member

@NicholasBoll NicholasBoll commented May 15, 2024

Summary

Add support for using parent class names in Stencil selectors to support sharing of modifiers to subcomponents. Allows support for #2711

Release Category

Styling


Overall, this change does the following things:

  • Remove the need for makeEmotionSafe
    • The hash is now the last part of the variables, so there's no more need for it to be Emotion safe
    • *-emotion-safe was being added to the CSS Kit output. CSS Kit doesn't use Emotion and doesn't need to be Emotion safe
  • Separate the names resolution cache from the extractedNames resolution cache
    • Variables resolved with the same value in React Kit and CSS Kit. This is not necessary and there's less chance of collisions if we use hashes in React Kit.
    • Before React Kit: --cnvs-button-label-emotion-safe
    • After React Kit: --label-button-{hash}
    • Before CSS Kit: --cnvs-button-label-emotion-safe
    • After CSS Kit: --cnvs-button-label
  • Add class names created by createStyles and createStencils to the names and extractedNames cache. This allows a reference to class names to be used in selectors.

For example, this is not possible before this change:

const formFieldStencil = createStencil({
  base: { /* ... */ },
  modifiers: {
    orientation: {
      horizontal: { /* ... */ },
      vertical: { /* ... */ }
    }
  }
})

const formFieldLabelStencil = createStencil({
  base: {
    // base styles
    [`.${formFieldStencil.modifiers.orientation.horizontal} :where(&)`] {
      // styles if the formFieldStencil has the horizontal orientation
    }
  }
})

This was possible at runtime, but the transform did not know how to handle it because the modifier class name wasn't resolved yet. This addition removes the need to share orientation with the FormField's model and have a modifier in formFieldLabel which would require CSS Kit authors to add the orientation modifier to both FormField AND FormField.Label.

Checklist

  • Label ready for review has been added to PR

For the Reviewer

  • PR title is short and descriptive
  • PR summary describes the change (Fixes/Resolves linked correctly)

@NicholasBoll NicholasBoll added the ready for review Code is ready for review label May 15, 2024
Copy link

cypress bot commented May 15, 2024

Passing run #7299 ↗︎

0 970 3 0 Flakiness 0

Details:

Merge 07fe9b8 into a0579b1...
Project: canvas-kit Commit: 3e62e72dd1 ℹ️
Status: Passed Duration: 05:02 💡
Started: May 15, 2024 5:33 PM Ended: May 15, 2024 5:38 PM

Review all test suite changes for PR #2741 ↗︎

@@ -1,6 +1,6 @@
{
"name": "@workday/canvas-kit-css",
"version": "10.3.24",
"version": "10.3.36",
Copy link
Member Author

Choose a reason for hiding this comment

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

The build process automates the syncing of these version strings.

Comment on lines +23 to +24
let vars: TransformerContext['names'] = {};
let extractedNames: TransformerContext['extractedNames'] = {};
Copy link
Member Author

Choose a reason for hiding this comment

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

names and extractedNames are used by the static resolution system. The transformation process populates these caches with resolved names.

For example:

const foo = createStencil({
  modifiers: {
    size: {
      small: {},
      large: {}
    }
  }
})

const bar = createStencil({
  base: {
    // The type of `foo.modifiers.size.small` is `string` which the static
    // transform doesn't like. It will now be added to `names` so that
    // static resolution uses the actual class name
    [`.${foo.modifiers.size.small} :where(&)`]: {
    }
  }
})

Copy link
Member Author

Choose a reason for hiding this comment

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

names are a cache of the resolved names for React Kit. It will include hashes. The extractedNames are the names used in CSS Kit.

for (const key in fallbackVars) {
names[key] = fallbackVars[key];
}
// vars = {...names, ...fallbackVars};
Copy link
Member Author

Choose a reason for hiding this comment

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

This can be removed

Comment on lines +88 to +91
// eslint-disable-next-line guard-for-in
for (const key in fallbackVars) {
names[key] = fallbackVars[key];
}
Copy link
Member Author

Choose a reason for hiding this comment

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

There was a bug where passing in names to the transform did not return the same reference. That's because a new reference was used internally. That means I couldn't properly test changes. This update instead mutates the name cache that was passed in so the cache can be properly tested against.

@NicholasBoll NicholasBoll added automerge and removed ready for review Code is ready for review labels May 15, 2024
@alanbsmith alanbsmith enabled auto-merge (squash) May 15, 2024 20:49
@alanbsmith alanbsmith merged commit 2313244 into Workday:prerelease/major May 15, 2024
19 checks passed
@NicholasBoll NicholasBoll deleted the feat/allow-stencil-modifiers-in-selectors branch May 15, 2024 20:57
alanbsmith pushed a commit that referenced this pull request May 16, 2024
PR #2741 added the ability to target parent modifier class names to build selectors, but doesn't work in compat mode because compat mode merges all the CSS class names into a single, new class name. I added a `parentModifiers` function that handles this for both React Kit and CSS Kit and updated stencils to add inert class names for the purpose of always matching a selector.

I also fixed a bug where passing `undefined` to a Stencil for a modifier key resulted in overriding `defaultModifiers`.

[category:Styling]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants