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

Features & Fixes: DOM in core, less DOM writes, N Composition, #450

Merged
merged 26 commits into from
Mar 8, 2021

Conversation

jonathantneal
Copy link
Contributor

@jonathantneal jonathantneal commented Mar 2, 2021

This PR would introduce the following changes:

  • Fixed issues composing together and passing along variants, compoundVariants, and defaultVariants.
  • Reduced how often the DOM is modified without reducing how quickly it is updated.
  • Added DOM injection to core.
  • Added the ability to create a component extended from an unlimited number of other components.
  • Changed the theme function to return an object instead of another function.

PR Goals

@jonathantneal jonathantneal requested a review from peduarte March 2, 2021 05:36
@codesandbox-ci
Copy link

codesandbox-ci bot commented Mar 2, 2021

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 2f211a1:

Sandbox Source
Stitches CI: CRA Configuration
Stitches CI: Next.js Configuration
Stitches: pass variants down to wrapped component Issue #416
eloquent-mcnulty-up1de Issue #431
Stitches Theme difference between APIs - Styled Version Issue #436
Stitches Theme difference between APIs - CSS Version Issue #436
stitches bug Issue #441

@peduarte
Copy link
Contributor

peduarte commented Mar 2, 2021

🔥

@brodycj brodycj mentioned this pull request Mar 2, 2021
@jonathantneal
Copy link
Contributor Author

I’ve removed #448 from the goals of this PR, because like #449 the issue acknowledges that there isn’t an obvious problem or solution to address, neither inside or outside of Stitches.

@jonathantneal jonathantneal marked this pull request as ready for review March 8, 2021 14:06
@jonathantneal jonathantneal merged commit be2734e into canary Mar 8, 2021
@jonathantneal jonathantneal deleted the jn.use-n-flow branch March 8, 2021 17:38
@arempe93
Copy link
Contributor

arempe93 commented Mar 8, 2021

hey @jonathantneal -- tried this branch in my project since it should fix #431, discovered a bug with defaultVariants and composed components

https://codesandbox.io/s/default-variants-bug-11qbb
TypeError: Cannot read property 'undefined' of undefined

I tracked it down to this line here:

https://github.com/modulz/stitches/blob/be2734e7e1ed7ddf00865c8d28855025beabd70b/packages/core/src/index.js#L243

because variantProps[defaultVariantName] could be undefined. So may be a simple fix to guard that, but I'm not aware of the potential second order effects with all the changes.

@jonathantneal
Copy link
Contributor Author

@arempe93, thank you 🙏! Slipped the fix into #456

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