[typescript] Allow lab components to have overrides in theme #21279
Conversation
Details of bundle changes.Comparing: 1f18002...bc62bb8 Details of page changes
|
oliviertassinari
added
package: icons
There was a problem hiding this comment.
@CarsonF Thanks for spending time on it :)
Wasn't sure the best place to put this within lab/. Happy to move.
Maybe in an overrides.d.ts file?
Should we handle theme.props at the same time that theme.overrides?
|
If this approach is successful, we should be able to apply it too for mui/material-ui-pickers#1794, cc @dmtrKovalenko |
|
@oliviertassinari I got the hint lol 😉
Yeah sure
Oh I forgot about that. Yeah we should. |
CarsonF
force-pushed
the
feature/lab-component-mapping
branch
from
7d62b68 to
56a84e4
Compare
|
It works perfectly for pickers
|
CarsonF
force-pushed
the
feature/lab-component-mapping
branch
from
56a84e4 to
98301c0
Compare
CarsonF
force-pushed
the
feature/lab-component-mapping
branch
from
98301c0 to
951b370
Compare
|
@eps1lon What would you like documented? Theme overrides are already documented... |
Sure |
CarsonF
force-pushed
the
feature/lab-component-mapping
branch
from
951b370 to
404f76d
Compare
I think the current tests only works incidentally. Those tests shouldn't work in isolation. How would typescript know to include |
|
Ah you're right. I thought I had checked that but it was a bad check on my part. Ok at some point in user's src they'll need to import those files import '@material-ui/lab/overrides';
import '@material-ui/lab/props';They should probably be re-exported from a single point for connivence. import '@material-ui/lab/theme';I don't think that will cause problems for tree shaking as long as it's from an import without any complied JS. Since the user will need to do something I can add it to the docs too. |
|
@oliviertassinari @eps1lon Thoughts on what you would like the import to be and where in the docs this should go? |
|
@CarsonF It seems that we have 4 options.
import '@material-ui/lab/overrides';
import '@material-ui/lab/props';
import '@material-ui/lab/theme';
What about we go with 4? Would it work with the lab and the pickers at the same time? Otherwise, maybe we should keep it to 1? |
|
I vote for 4 as well. There's caveats though. TS will only pick up the overrides if there's an import for it in the users src files. So we could include the overrides in import { AvatarGroup } from '@material-ui/lab'; // overrides in index.ts so TS picks them up
import AvatarGroup from '@material-ui/lab/AvatarGroup'; // overrides are missedI always import from root and let the build chain figure out tree shaking which seems to work fine for me, but idk about others. If we wanted to support multiple different import paths, we would need to do the overrides individually at that level. // src/AvatarGroup/AvatarGroup.d.ts
...
declare module '@material-ui/core/styles/overrides' {
interface ComponentNameToClassKey {
MuiAvatarGroup: AvatarGroupClassKey;
}
}
declare module '@material-ui/core/styles/props' {
interface ComponentsPropsList {
MuiAvatarGroup: AvatarGroupProps;
}
}This still could be the best solution as it would cover all cases automatically and keeps logic closer to the component. But the boilerplate for every component isn't nothing. |
|
What about the overrides for multiple packages: lab, pickers, x (soon), etc.? For the import issue, can't the individual components be written so they import the theme.d.ts file (no need to cherry-picking)? |
|
Yeah if you wanted to keep the overrides in a single place and then imported them for every component that would work too. // AvatarGroup
import '../overrides';
// TabList
import '../overrides';
// overrides
import './AvatarGroup';
import './TabList';If this wasn't just for TS types I would be worried about circular dependencies, but since there is no runtime code here it will be fine. I'm not sure what you mean for multiple packages. The pattern we decide here can be applied to every non-core package. |
Do we have a precedent in the codebase for a kebab-case filename? It seems that camelCase for modules and PascalCase for components is the current convention. |
|
Looks like not, other than a couple edge cases. types could left out... import type '@material-ui/lab/theme';I know that we are still supporting TS 3.2, but there's no real reason not to upgrade to latest TS for users (unless I'm missing something...). We could document it this way or at least document both. This should give enough clarity and could leave room for actual logic in this theme folder in the future... |
|
In this case, my vote would go for something close to: import type '@material-ui/lab/themeAugmentation';or even import type '@material-ui/lab/themeComponentsAugmentation';There is no need for the import name to be short, the more details, the better. |
CarsonF
force-pushed
the
feature/lab-component-mapping
branch
from
79e4fd5 to
7dbb7a1
Compare
I'm bad about that 😉 Went with |
I prefer that. |
|
Regarding the documentation, should we have it covered in https://material-ui.com/components/about-the-lab/? |
|
Yeah we can add something there. I can get to it later tonight if someone doesn't beat me to it. |
|
@CarsonF I'm giving the documentation a try. |
oliviertassinari
force-pushed
the
feature/lab-component-mapping
branch
from
7efa767 to
0002d52
Compare
Co-authored-by: Sebastian Silbermann <[email protected]>
Co-authored-by: Carson Full <[email protected]>
| @@ -0,0 +1,2 @@ | |||
| export * from './overrides'; | |||
There was a problem hiding this comment.
this files is not exported in the main index.d.ts
There was a problem hiding this comment.
It was decided above that that would be too magical for users and that an explicit import for themeAugmentation would be better.
There was a problem hiding this comment.
orite, makes sense. How exactly should we use this
e.g. this is my current theme file theme.tsx
import createMuiTheme from "@material-ui/core/styles/createMuiTheme";
import type '@material-ui/lab/themeAugmentation'; // error: '=' expected.ts(1005)
export const theme = createMuiTheme({
overrides: {
// MuiAlert: {},
}
})There was a problem hiding this comment.
I am using create-react-app BTW
There was a problem hiding this comment.
Tried this
import type * as LabTypes from '@material-ui/lab/themeAugmentation';
worked but eslint is complaining that
'LabTypes' is declared but its value is never read.ts(6133)
There was a problem hiding this comment.
Thank you very much. very helpful.
I googled so much but cannot find a best way for this.
What is the way to tell makeStyles that you are only allowed to use Alert classes? The following works but I am sure there must be a better way.
import { Theme, makeStyles } from "@material-ui/core";
import { AlertClassKey } from "@material-ui/lab/Alert";
export const useAlertClasses = makeStyles<Theme>(
(): Partial<Record<AlertClassKey, any>> => ({
root: {
borderRadius: 3,
}
}));
There was a problem hiding this comment.
@CarsonF I'm facing the same issue. I'm also using create-react-app. Already made sure that all sub-dependencies are up to date by using yarn upgrade react-scripts. I'm using TypeScript 3.8.3.
Named type imports are working completely fine but not these unnamed ones.
There was a problem hiding this comment.
It should be fixed in v5.0.0-alpha.3 with #21724
There was a problem hiding this comment.
@oliviertassinari Could you backport this fix to the 4.x line of labs? I don't want to upgrade to 5.x yet.
There was a problem hiding this comment.
@levrik For now, you can copy & paste the diff of this pull request
|
This does not seem to work for CRA. Can you verify that? |
|
@Domino987 It should be fixed in v5.0.0-alpha.3 with #21724 |
oliviertassinari
changed the title
Closes #19427
Wasn't sure the best place to put this within lab/. Happy to move.
/cc @oliviertassinari @eps1lon