-
-
Notifications
You must be signed in to change notification settings - Fork 38
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
Requirements for making tss-react compatible for Material-UI makeStyles replacement #3
Comments
This is a tiny but helpful package. It would be great if we can integrate it as one of the default style options in MUI, applying exactly the same interface as what JSS supports in V4 and keep maintenance the code within MUI repo. |
Hi @mnajdova,
I haven't tested it yet. I can make time for that on the week-end. Is this still true?
Yes, it unfortunately is necessary in order to get the type inference working. createStyles<"root" | "label", { color: string; }>({
"root": {
"backgroundColor": "blue"
},
"label": ({ color })=>({
color
})
}) We shouldn't have to explicitly provide
Well, I am very passionate about this topic. Let me make my case and if you aren't convinced I'll make the API changes, even if I think it's a mistake, because I'd like this colab to go through. First, I believe it's almost always better to return results of functions wrapped into an object, even if there is only one result. The reasoning behind that is that a large part of a developer's job is to give meaningful and consistent names to things. Maybe it is more critical for me than for others because I am dyslexic but I know I am knot the only one for whom this pattern is life changing. Beside I am convinced that we should ditch the old naming scheme: Best regards |
I will set up a simple repository to test this out, and we can see if there are some problems. From the initial try on the Material-UI docs it did not work, but it would be simpler to reproduce on a clean slate.
Yes
It's unfortunate that the API needs to suffer because of TypeScript limitation :( In Material-UI we have a similar Regarding the API changes :)
I understand your point, looking from Material-UI perspective, if the API stays as it is at the current moment, we will need to create an adapter so that we can keep the same API as in v4. We are considering adding back the If there are other reasons for adding adapter, then it may be fine, but int he end it would be great if we can keep the same API :) Will link here later today/tomorrow the repository where we can test the integration. |
OK there is something that I want to point out.
const { useClassNames } = createUseClassNames<{ color: string; }>()(
(theme, { color })=> ({
"root": {
"backgroundColor": theme.palette.primary.main
},
"label": { color }
})
);
const useStyles = makeStyles(
theme => createStyles<"root" | "label">, { color: string; }>({
"root": {
"backgroundColor": theme.palette.primary.main
},
"label": ({ color })=>({
color
})
})
); Notice how with TSS we get
Regardless, I think we should optimize the user experience for TypeScript, not for vanilla JS.
...we can't, it's technically impossible as of TypeScript v3.4.5
Actually, there is one. Currently unlike JSS the Best regards. |
We now have a polyfill the Focusing on getting things to work on SSR. @mnajdova you have been of great help so far. Best regard |
@garronej I feel like we can close the issue, I don't see some blockers at this moment. As mention in mui/material-ui#26571 (comment) we are going to promote |
No, it's not right to return always an object when we want to return a single value; IMO this is a bad practise. End users must have a direct freedom to name things as they want; we should not create things just having in mind beginners that will make up weird names, teams and experienced developers will always be consistent regarding the names they're using. A library shouldn't care what internal names a project shall use. Most users and especially beginners, will always read the documentation when they'll start to learn how to develop (using MUI for example) so they'll see what naming convention the framework is using. An other awkward thing about this, is when we want to create multiple const { useClassNames: useComp1ClassNames } = createUseClassNames(...)
const { useClassNames: useComp2ClassNames } = createUseClassNames(...)
const { useClassNames: useComp3ClassNames } = createUseClassNames(...) instead of: const useComp1ClassNames = createUseClassNames(...)
const useComp2ClassNames = createUseClassNames(...)
const useComp3ClassNames = createUseClassNames(...) Libraries should not be opinionated regarding naming conventions and should not enforce any king of end naming. |
Well... we have conflicting views. That's said, I am willing to make the following API changes if there is a clear consensus in this direction. -const { useStyles } = makeStyles()(...);
+const useStyles = makeStyles()(...); @clytras, on a personal note, your peremptory tone did rub me the wrong way. Feel free to express your opinion, it is taken into consideration but please, be polite. |
@garronej what exactly wasn't polite and with a "peremptory tone" regarding my comment? That I told you that "it's not right to return always an object when we want to return a single value" or "IMO this is a bad practise"? I'm trying to make you rethink of something that it's obsiously wrong; Yeah, maybe I should have written "please reconsider the usage of this BAD practise" but I believe you're getting the point and I have no intention to insult you nor offend you in any way. It's not a hook, it is a function that it creates a hook and that makes a huge difference because it's very likely that there might be multiple hooks in the same file, it's not a standard hook like There are cases that we need to return an object even if we have a single value to return, like for example the
Developers won't have to wonder when they see how it's done in the examples and tutorials which they will dive, read and follow when they are about to learn something. IMO creating such an opinionated behaviour only adds syntactic noise and it doesn't add much regarding DX.
It is awkward when I want to create 3, 4 and many hooks to destructure a repetitive name like this; remeber here we don't have a standard hook, but we're creating new hooks and there might be many of them. Please reconsider the usage of this and please don't take my words personal; I don't know you, and there is no intention for any offence. |
I shouldn't have took your words personally, as I said I am very passionate about this issue. 😅 It's not my place to assert how the lib should be used maybe you have a valid use case where it make sense to declare multiple Also, I stress that named return is an accessibility features. We are not all equals regarding what we can remember, a lib that only requires to remember concepts and not naming conventions is way more friendly to people with ADHD and beginners. That said your point was upvoted twice, @mnajdova also suggested this change and it was like that in |
This PR https://github.com/DimensionDev/Maskbook/pull/3745/files gives an interesting overview of the feasibility to move from |
Following up on our discussion started in mui/material-ui#26571 about how we can make
tss-react
compatible as a replacement for themakeStyles
API for v5. I've done some initial testing. Here are some thoughts:SSR
Have you tried to configure nextjs to work with
tss-react
? It uses @emotion/css so I believe some extra steps would be required. This would be the first requirement so that we could use it together with material-ui. Our docs (the Material-UI's) can be a good candidate for testing this out.API suggestions
Here are some suggestions regarding the API itself:
I wouldn't return named options, if there is only one option returned. For example:
With this, we can actually make the API closer to the v4
makeStyles
API. We could overcome this by adding adapters, but I will leave it to you.Question: Is it really necessary to invoke function
createUseClassNames()
and then propagate the input to the next function call? What are the arguments required there? Can it be omitted?I am open to help and move this forward :)
The text was updated successfully, but these errors were encountered: