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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Can't augment common colors of theme palette anymore #20180

Closed
levrik opened this issue Mar 19, 2020 · 17 comments
Closed

Can't augment common colors of theme palette anymore #20180

levrik opened this issue Mar 19, 2020 · 17 comments
Assignees

Comments

@levrik
Copy link

@levrik levrik commented Mar 19, 2020

  • The issue is present in the latest release.
  • I have searched the issues of this repository and believe that this is not a duplicate.

Current Behavior 馃槸

I couldn't find a way to augment the common key of a Palette anymore.

Expected Behavior 馃

That it is possible like before.

Steps to Reproduce 馃暪

Material-UI 4.9.5: https://codesandbox.io/s/peaceful-hodgkin-i05mi (working)
Material-UI 4.9.6: https://codesandbox.io/s/nameless-sun-r7nrv (not working)

Context 馃敠

After upgrade to 4.9.6 of Material UI our builds started to break.

In version 4.9.6 the common colors are exported from @material-ui/core/colors/common as default and with their name CommonColors as well. This made it possible to augment the types.
Sadly TypeScript doesn't allow augmentation of default exports. That's why it isn't possible in 4.9.6 anymore since CommonColors is gone and a const named common is just exported as default.
Please see microsoft/TypeScript#14080 as reference for the mentioned TypeScript issue.

I also can't augment Palette directly since with augmentation we can't redefine an already defined key of the Palette.
I'm also wondering why the interface was removed. Before there was the interface, a type variable called common and the default export. By just getting the interface back and exported by its name like before this issue would be fixed.

Your Environment 馃寧

Tech Version
Material-UI v4.9.6
TypeScript 4.8
@oliviertassinari

This comment has been minimized.

Copy link
Member

@oliviertassinari oliviertassinari commented Mar 19, 2020

Note that import createPalette from "@material-ui/core/styles/createPalette"; isn't a legit import, this module is private.

@oliviertassinari

This comment has been minimized.

Copy link
Member

@oliviertassinari oliviertassinari commented Mar 19, 2020

@levrik From what I understand, we can close this issue, the augmentation should happen at the theme level, not the color one.

@levrik

This comment has been minimized.

Copy link
Author

@levrik levrik commented Mar 19, 2020

@oliviertassinari For some reason I got a createPalette is not a function error when I did

import { createPalette } from '@material-ui/core/styles';
@levrik

This comment has been minimized.

Copy link
Author

@levrik levrik commented Mar 19, 2020

@oliviertassinari No. Not really. As explained in my description the augmentation from the Theme/Palette level doesn't work. I have to be able to augment the common colors definition directly.

@oliviertassinari

This comment has been minimized.

Copy link
Member

@oliviertassinari oliviertassinari commented Mar 19, 2020

createPalette is private

@eps1lon

This comment has been minimized.

Copy link
Member

@eps1lon eps1lon commented Mar 19, 2020

You can't augment core/colors anyway. If you patch your local node_modules/ then you're on your own.

Maybe you tried to augment colors the Theme? https://material-ui.com/guides/typescript/#customization-of-theme should help you achieve that.

@levrik

This comment has been minimized.

Copy link
Author

@levrik levrik commented Mar 19, 2020

@oliviertassinari That's interesting. Because TypeScript didn't complain about the import of createPalette from @material-ui/core/styles.

@levrik

This comment has been minimized.

Copy link
Author

@levrik levrik commented Mar 19, 2020

@eps1lon Yep. We've started on this guide but thought that a common blue color used throughout our application makes the most sense inside the common key of the palette. If I remember correctly for example Box can only use colors from the palette as bgcolor, not from the theme directly. It was a requirement for us to be able to use this common color for Boxes.

Also want to note that we never patched node_modules/. We were just augmenting the TypeScript types for something that was possible to change just from the outside.

@eps1lon

This comment has been minimized.

Copy link
Member

@eps1lon eps1lon commented Mar 19, 2020

Also want to note that we never patched node_modules/. We were just augmenting the TypeScript types for something that was possible to change just from the outside.

You should only augment what you actually change. In your case you told typescript that you somehow changed import { common } from '@material-ui/core/colors'. You probably didn't use this in your codebase to begin with which made the type change harmless. It's nevertheless unsound if you don't actually patch the runtime.

Augmenting the theme is less harmful since we assume that you change the context value accordingly.

@levrik

This comment has been minimized.

Copy link
Author

@levrik levrik commented Mar 19, 2020

@eps1lon Please take a look at the first Codesandbox I've posted.
That's basically exactly what we're doing. We've passed a custom palette. So we augmented what we've changed. Without the augmentation our code is broken.
Whatever. Could you suggest an alternative? As I said it seems like the Box component is reading from the palette. We want to provide a custom color through the theme to be used by Box. That's the whole reason why we started to change the palette. As long as there's no way to use colors directly defined on a theme from a Box we probably have to stay on 4.9.5. Which is something I don't want to.

@eps1lon

This comment has been minimized.

Copy link
Member

@eps1lon eps1lon commented Mar 19, 2020

So we augmented what we've changed

No, you changed colors/common which is also used by the theme when you only actually augmented the theme.

Could you suggest an alternative?

The guide I linked exactly described this. If you have further question then stackoverflow is the better place for these kind of issues.

@eps1lon eps1lon closed this Mar 19, 2020
@levrik

This comment has been minimized.

Copy link
Author

@levrik levrik commented Mar 19, 2020

@eps1lon No. The guide doesn't answer the question about the Box component.
So. How can I change the palette of a theme? Is this an unsupported thing? Because it works. But I can't make it work with TypeScript. This is basically what this issue is about.
With augmentation I can't change the type of a property of Theme. I can't augment the palette key. Augmentation works for adding fields only. Not for altering the type of existing ones.

@oliviertassinari

This comment has been minimized.

Copy link
Member

@oliviertassinari oliviertassinari commented Mar 19, 2020

How can I change the palette of a theme?

@levrik While the generic approach is documented in https://material-ui.com/guides/typescript/#customization-of-theme, I have tried to update your codesandbox with no luck. I don't know how to process, but I believe it's possible. Maybe it would be great to have a demo for it?


TypeScript didn't complain about the import of createPalette from @material-ui/core/styles.

Great catch, it sounds like we should keep the types in sync with the implementation. I believe this diff would do it :).

diff --git a/packages/material-ui/src/styles/index.d.ts b/packages/material-ui/src/styles/index.d.ts
index febaa6212..7414c1adc 100644
--- a/packages/material-ui/src/styles/index.d.ts
+++ b/packages/material-ui/src/styles/index.d.ts
@@ -1,10 +1,6 @@
 export * from './colorManipulator';
 export { default as createMuiTheme, ThemeOptions, Theme, Direction } from './createMuiTheme';
-export {
-  default as createPalette,
-  PaletteColorOptions,
-  SimplePaletteColorOptions,
-} from './createPalette';
+export { PaletteColorOptions, SimplePaletteColorOptions } from './createPalette';
 export { default as createStyles } from './createStyles';
 export { TypographyStyle, Variant as TypographyVariant } from './createTypography';
 export { default as makeStyles } from './makeStyles';

Do you want to send a pull request? :)

@hburrows

This comment has been minimized.

Copy link
Contributor

@hburrows hburrows commented Mar 21, 2020

@eps1lon I'm a little confused by what you're proposing the solution to this problem is. Are you saying we should just add a new property to Palette (for example, my_common)? As @levrik is saying, the way CommonColors is now defined you can't augment the common property of Palette -- it's private and it's defined as a type equal to Record<...> and you can't change the type via augmentation (so we can't define the type as Record<white | black | lightBlack | darkWhite | ...>. If that's the case why even have a common property in Palette which only has white (#fff) and black (#000) -- how often does one use pure white or black in a design.

@oliviertassinari

This comment has been minimized.

Copy link
Member

@oliviertassinari oliviertassinari commented Mar 21, 2020

You should only augment what you actually change. In your case you told typescript that you somehow changed import { common } from '@material-ui/core/colors'.

@hburrows This is important, the type should match reality 猡达笍.

@hburrows

This comment has been minimized.

Copy link
Contributor

@hburrows hburrows commented Mar 21, 2020

@oliviertassinari That makes sense. Unless I'm missing something obvious that means there is no way to add additional colors to Palette.common. However, if that is possible, can you please share an example because it's lost on me. I'm OK adding a sibling to Palette.common (i.e. Palette.app_common) to hold my app's common colors, but I just don't want to do that if I'm not understanding this. It seemed like adding my app's common colors into Palette.common via Theme customization was the way to go. Again, if you can't do that then having a Palette.common with only black and white has zero value.

@eps1lon

This comment has been minimized.

Copy link
Member

@eps1lon eps1lon commented Mar 21, 2020

I was under the impression you could just add deep properties with module augmentation but seems like I misunderstood something. We'll add the ability to augment this.

Quite unfortunate that this just happened to work by augmenting colors/common. Sorry for the trouble.

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

Successfully merging a pull request may close this issue.

4 participants
You can鈥檛 perform that action at this time.