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

Docs: Document VarGroup and Theme types #431

Merged
merged 3 commits into from
Feb 11, 2024
Merged

Docs: Document VarGroup and Theme types #431

merged 3 commits into from
Feb 11, 2024

Conversation

nmn
Copy link
Contributor

@nmn nmn commented Feb 8, 2024

What changed / motivation ?

Document public types VarGroup and Theme. The writing could probably be better,
but sharing early for feedback.

Linked PR/Issues

Fixes #424

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Feb 8, 2024
Copy link

github-actions bot commented Feb 8, 2024

compressed-size: runtime library

Size change: 0.00 kB
Total size: 2.52 kB

View unchanged
Filename: gzip (minify) kB size kB change % change
./packages/stylex/lib/stylex.js 1.04 (3.22) 0.00 (0.00) 0.0% (0.0%)
./packages/stylex/lib/StyleXSheet.js 1.48 (3.75) 0.00 (0.00) 0.0% (0.0%)

Copy link

github-actions bot commented Feb 8, 2024

compressed-size: e2e bundles

Size change: 0.00 kB
Total size: 1128.55 kB

View unchanged
Filename: gzip (minify) kB size kB change % change
./apps/rollup-example/.build/bundle.js 1005.20 (10185.36) 0.00 (0.00) 0.0% (0.0%)
./apps/rollup-example/.build/stylex.css 123.34 (773.82) 0.00 (0.00) 0.0% (0.0%)

@TxHawks
Copy link

TxHawks commented Feb 8, 2024

I feel that the description and explanation of the second type argument to both VarGroup and Theme isn't clear enough, and that the example doesn't clearly illustrates what it would be used for, or what's a real world use case in which you'd reach out for this.

Otherwise it's all very clear and well articulated

@NoriSte
Copy link

NoriSte commented Feb 8, 2024

Thanks for working on it 😊 IMO

  1. The doc is clear
  2. The doc is concise
  3. The last part should be explained better

Screenshot_20240208-053333

What I mean: the users could miss the use case and stuck on it because that want to fully understand it. If they can't, an idea could pop in their mind: "stylex is hard". And it's a lose-lose.

My Pareto-principle suggestion here is: just mention that "this technique is commonly referred as Branded Types" so people have something to Google for. And if the technique doesn't click for them, the result is that in their mind "oh, TypeScript is hard, but stylex is cool" pops 😊

@nmn
Copy link
Contributor Author

nmn commented Feb 8, 2024

@NoriSte @TxHawks Thanks for the feedback! The challenge is that the second argument should be needed very rarely. Your feedback has been very helpful. I'll think about how to both make it easier while making it evident that's it's an advanced feature that shouldn't be needed most of the time.

@TxHawks
Copy link

TxHawks commented Feb 8, 2024

I think giving a concrete example of when it would be needed would help in both explaining exactly what it is, and that it's an advanced feature that should hardly ever be used.

Maybe something along the lines of

The second type argument should be rarely needed, and is meant for advanced cases where type branding might be desirable.

For example, ... 

@nmn
Copy link
Contributor Author

nmn commented Feb 10, 2024

@TxHawks @NoriSte I've updated the write up. Please see this link to see if it's both clearer and less overwhelming.

I removed the second argument from the core explaination and put it in a <details> element.

@NoriSte
Copy link

NoriSte commented Feb 10, 2024

Well done!! 😊

@TxHawks
Copy link

TxHawks commented Feb 10, 2024

This makes it completely clear in my opinion. There's a small typo though:
However, each usage of stylex.defineVars

The of us missing

@nmn nmn merged commit c22d101 into main Feb 11, 2024
9 checks passed
@nmn nmn deleted the docs/varGroup-types branch February 11, 2024 11:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Document types for StyleX theming
4 participants