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

[pigment-css][docs] Pigment CSS README copyediting #41838

Draft
wants to merge 7 commits into
base: next
Choose a base branch
from

Conversation

samuelsycamore
Copy link
Member

follow-up to #41762 - not yet ready for review

@samuelsycamore samuelsycamore added docs Improvements or additions to the documentation package: pigment-css Specific to @pigment-css/* labels Apr 10, 2024
Comment on lines +874 to +875
'--x': (props) => props.x,
'--y': (props) => props.y,
Copy link
Member Author

@samuelsycamore samuelsycamore Apr 10, 2024

Choose a reason for hiding this comment

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

@aarongarciah aarongarciah 2 days ago
I think this is the first reference to props => x in the README. I'm a bit confused about this, let me try to clarify why:

Above, we have two sections:

"Styling based on props" -> we explain this approach works great when values are known at build time. Gives the impression that accessing the props object is not possible in styled.
"Styling based on runtime values" -> we explain two possibilities to handle runtime values:
Using hand-written inline CSS vars.
Using a callback: Pigment will transform it to an inline CSS var and inline the return value of the callback so it runs at runtime.
This section, "Programmatically generated styles", seems like another use case of "Styling based on runtime values". I expected to see one of the two solutions named above (inline CSS vars or callbacks). Instead, we say accessing props is possible, but the rest of the README gives the impression that it's not possible.

If using props => value callbacks is possible, should we document it in the "Styling based on runtime values" section above, too?

(I know this part is not introduced by your PR, but I think it's probably a good moment to fix it if something is confusing)

Member
Author
@samuelsycamore samuelsycamore yesterday
cc @brijeshb42 @siriwatknp I would love to hear your thoughts on @aarongarciah 's comment here!

Member
@brijeshb42 brijeshb42 8 hours ago
We've already mentioned accessing props in the section Using a callback that you referenced. It's the same thing except there, we are destructuring isError, so there is no direct mention of props. But it's the same thing. See line 333.

Member
@aarongarciah aarongarciah 7 hours ago
Oh, I see! I think more people will miss this. Maybe we can improve the "Using a callback" part to be more explicit about being able to access props:

  • Reword the explanation under "Use a callback function as a value ..." so we explicitly mention that callbacks have access to props.
  • Do not destructure props in the example so it's clear what's going on.
    const Heading = styled('h1')({
    -  color: ({ isError }) => (isError ? 'red' : 'black'),,
    +  color: (props) => (props.isError ? 'red' : 'black'),
    });
  • Maybe put "Option 2 – Use a callback ..." before "Option 1 – Declare a CSS variable...". I think styling based on props is probably a more widespread use case than creating custom CSS vars.

Food for thought: right now, we make the distinction between "props" (in the "Styling based on props" section) and "runtime values" (in the "Styling based on runtime values"), but maybe the distinction could be better conveyed with "static" (build time) vs "dynamic" values (runtime), because runtime styling can also be based on props via callbacks.

@mui-bot
Copy link

mui-bot commented Apr 10, 2024

Netlify deploy preview

https://deploy-preview-41838--material-ui.netlify.app/

Bundle size report

No bundle size changes (Toolpad)
No bundle size changes

Generated by 🚫 dangerJS against 4b66eda

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Apr 13, 2024
@oliviertassinari
Copy link
Member

oliviertassinari commented Apr 14, 2024

I guess we should close this PR and reopen it in the new repository. For example, the community started fixing the ones Sam and I talked about in mui/pigment-css#5 (the ones that using Grammarly IDE extension would prevent cc @siriwatknp)

@siriwatknp
Copy link
Member

I guess we should close this PR and reopen it in the new repository. For example, the community started fixing the ones Sam and I talked about in mui/pigment-css#5 (the ones that using Grammarly IDE extension would prevent cc @siriwatknp)

Agree. The new repo is https://github.com/mui/pigment-css

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Improvements or additions to the documentation package: pigment-css Specific to @pigment-css/* PR: out-of-date The pull request has merge conflicts and can't be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants