BaseTheme default values.

[alitaheri]

Without default values for raw themes many things will break. I think ThemeManager should deep assign the raw theme provided by user onto the default raw theme to prevent regressions like #2459. It will also make it easier to safely add options to raw themes, options we will have to add in the future. I had the same issue when I did an update.

@oliviertassinari Thoughts?

[oliviertassinari]

had the same issue when I did an update

Same here. I think that we should improve getMuiTheme().
I think that it should take two parameters, a rawTheme and a muiTheme.
We whould

• do a deep merge between the rawTheme parameter and a rawThemeDefault.
• use the output to compute a muiThemeComputed
• do a deep merge between the muiThemeComputed and the muiTheme parameter
• return the result

[alitaheri]

@oliviertassinari I really like your idea, and both can be optional! oh and we should remember to add isRtl to one of them. which one do you think is better for this property?

[oliviertassinari]

Where shoud we put the isRtl?
IMHO, the rawTheme should contain variables that are needed for different properties of the muiTheme. Hence, I would say in the muiTheme.

[alitaheri]

@oliviertassinari But aren't some style information generated taking into account the isRtl prop?

[oliviertassinari]

Yes, but that's latter down the pip.

rawTheme -> muiTheme -> style -> style with a direction -> style with a direction and prefixed

[alitaheri]

Ooo I see. alright then, tnx :D

[mbrookes]

If you're going for a re-write, any chance to rename raw-theme to base-theme or core-theme or similar? I found that naming really confusing when 0.13 was introduced.

Even though the "raw" themes are only the base values for theme-manager, conceptually they are the theme, as (other than custom themes) it what the user specifies to choose a theme.

To me, raw says "unpopulated", "incomplete", "fill-in-the-blanks", which isn't the case.

[alitaheri]

@mbrookes Yeah, can't say I don't agree with you on this. But this breaking change will be painful, just search rawTheme in the source :D I don't know if it's worth the effort, @oliviertassinari Thoughts?

[oliviertassinari]

I agree with @mbrookes, baseTheme seems like a better name for this object.
But yes, that's would be a breaking changes, people would have to change their import.

[mbrookes]

Could you perhaps retain the "raw" theme files as stubs that import the new baseTheme, so the old names would still work?

Regarding @subjectix point about number of usages - internally if the name rawTheme was still used it wouldn't matter so much if the documented / end-user visible name were change. But a global search-and-replace would easily fix it! 😄

[alitaheri]

@mbrookes good point. Maybe we can push this into the 0.14.0 release. But right now it's almost impossible to just do a global scan/replace, too many Big changes are occurring = too many conflicts will arise. we'll have to wait until the docs are migrated :D

[mbrookes]

Oh, yes. Didn't think about merge conflicts.

[alitaheri]

More reason why we have to implement this #2529.

[mbrookes]

Thought I'd have a crack at implementing the base theme change: mbrookes@b92af82

I haven't changed the import for DefaultRawTheme in any of the components yet, just the docs site as a POC.

I removed the LightRawTheme and DarkRawTheme requires in styles/index.js just to prove they weren't being used, but would they need to be retained for backwards compatibility? (They aren't mentioned in the docs, so not sure...)

Let me know whether you think this is worth pursuing. I don't think it conflicts with the other proposed theme changes.

[newoga]

@mbrookes It seems like everyone likes the term baseTheme more, myself included 😄. I made a comment on your commit but I think you can open a new pull request as this issue seems to be more about deep merging defaults for themes. It'll be easier to review your changes as a new pull request and material-ui build/linter will run too.

[mbrookes]

Yes, sorry to hijack this issue with mine. The deep merge work is much more interesting & important!

I'll open a PR as [WIP], and no problem if it gets bounced as not the right direction for the project.

[alitaheri]

@mbrookes baseTheme is definitely a much better name. rawTheme confused me at first too. 👍 👍

[alitaheri]

@oliviertassinari I'm working on this. don't touch it :D

[oliviertassinari]

Can we close this issue? (#2635)

[alitaheri]

@oliviertassinari Yes absolutely 😁