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.

Sign up for GitHub

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

Jump to bottom

[docs] Include JSS in styling solution interoperability guide #30736

Merged
merged 5 commits into from
Feb 7, 2022
Merged

[docs] Include JSS in styling solution interoperability guide #30736

merged 5 commits into from
Feb 7, 2022

Conversation

garronej
Copy link
Contributor

Good day,

I think the JSS hook-based API (makeStyles -> useStyles) has still a solid fanbase.
If you think that this is pushing TSS too hard, feel free to close, I won't take offense.

Best regads,

@mui-bot
Copy link

mui-bot commented Jan 22, 2022 •
edited
Loading

No bundle size changes

Generated by 🚫 dangerJS against 652e29b

@github-actions github-actions bot added PR: out-of-date The pull request has merge conflicts and can't be merged and removed PR: out-of-date The pull request has merge conflicts and can't be merged labels Jan 24, 2022
@danilo-leal danilo-leal changed the title Include JSS in styling solution interoperability guide? [docs] Include JSS in styling solution interoperability guide Jan 24, 2022
@danilo-leal danilo-leal added the docs Improvements or additions to the documentation label Jan 24, 2022
mnajdova
mnajdova reviewed Jan 25, 2022
View reviewed changes
docs/src/pages/guides/interoperability/interoperability.md Outdated
@@ -734,3 +737,103 @@ export default function SliderThumbOverrides() {
return <Slider className="p-4" classes={{ active: 'shadow-none' }} />;
}
```

## JSS
Copy link
Member

Choose a reason for hiding this comment

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

I don't mind having section about tss-react, but having it under JSS header is misleading, as it is not using JSS, but emotion. I would rather add the usage of tss-react in a subsection under emotion maybe (I am not sure about this). cc @mui-org/core

Copy link
Contributor Author

@garronej garronej Jan 25, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Hi @mnajdova,

I am afraid that mentioning TSS as a subsection of @emotion could be confusing for some peoples.
The fact that TSS uses emotion under the hood is merely a technicality. The API of tss-react is much closer to react-jss than it is from the API of emotion.

On the other hand, I think that peoples that would like to use a JSS like API would be interested to know that there is an option for them.

What do you think of my update? I replaced the title by: JSS TSS.

Best regards,

PS: tss-react now has a proper documentation website: https://docs.tss-react.dev

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 31, 2022
@siriwatknp
Copy link
Member

👋 The migration PR has been merged.

Please follow these steps to make sure that the contents are all updated. (Sorry for the inconvenience)

  1. pull latest master from upstream to your branch
  2. if your PR has changes on the *.md or demo files, you are likely to encounter conflict because all of them have been moved to the new folder.
    2.1 revert the change on those markdown files you did
    2.2 pull latest master from upstream (you should not see conflict)
    2.3 make the changes again at docs/data/material/*
  3. run yarn docs:api
    • you might see the changes in docs/pages/material/api/* if your PR touches some of the components
    • it is okay if there is no changes

If you are struggle with the steps above, feel free to tag @siriwatknp

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Feb 5, 2022
@garronej
Copy link
Contributor Author

garronej commented Feb 5, 2022

It's done 👍

siriwatknp
siriwatknp requested changes Feb 7, 2022
View reviewed changes
Copy link
Member

@siriwatknp siriwatknp left a comment

Choose a reason for hiding this comment

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

Can you apply the suggested fix and run yarn prettier, thanks

garronej and others added 2 commits February 7, 2022 05:17
@garronej @siriwatknp
Update docs/data/material/guides/interoperability/interoperability.md
a8ae404 
Co-authored-by: Siriwat K <siriwatkunaporn@gmail.com>
@garronej
fmt
ca5db7e
@garronej
Copy link
Contributor Author

garronej commented Feb 7, 2022

Can you apply the suggested fix and run yarn prettier, thanks

done! thx

siriwatknp
siriwatknp approved these changes Feb 7, 2022
View reviewed changes
Copy link
Member

@siriwatknp siriwatknp left a comment

Choose a reason for hiding this comment

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

👍 Thanks for making TSS!

@siriwatknp siriwatknp merged commit 3873b15 into mui:master Feb 7, 2022
oliviertassinari
oliviertassinari reviewed Feb 20, 2022
View reviewed changes
docs/data/material/guides/interoperability/interoperability.md
> it's a peer dependency of `@mui/material`.

> ⚠️ For [Storybook](https://storybook.js.org): As of writing this lines storybook still uses by default emotion 10.
> Material-ui and TSS runs emotion 11 so there is [some changes](https://github.com/garronej/onyxia-ui/blob/324de62248074582b227e584c53fb2e123f5325f/.storybook/main.js#L31-L32)
Copy link
Member

@oliviertassinari oliviertassinari Feb 20, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
> Material-ui and TSS runs emotion 11 so there is [some changes](https://github.com/garronej/onyxia-ui/blob/324de62248074582b227e584c53fb2e123f5325f/.storybook/main.js#L31-L32)
> Material UI and TSS runs emotion 11 so there is [some changes](https://github.com/garronej/onyxia-ui/blob/324de62248074582b227e584c53fb2e123f5325f/.storybook/main.js#L31-L32)

I took care of it.

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

@oliviertassinari oliviertassinari oliviertassinari left review comments

@mnajdova mnajdova mnajdova left review comments

@siriwatknp siriwatknp siriwatknp approved these changes

Assignees
No one assigned
Labels
docs Improvements or additions to the documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@garronej @mui-bot @siriwatknp @oliviertassinari @mnajdova @danilo-leal