Skip to content

[WIP] Base themes #2541

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

Closed
wants to merge 5 commits into from
Closed

[WIP] Base themes #2541

wants to merge 5 commits into from

Conversation

mbrookes
Copy link
Member

Work-in-progress to evaluate the impact of transitioning the naming of raw themes to base themes.

import ColorManipulator from '../../utils/color-manipulator';
import Spacing from '../spacing';
import zIndex from '../zIndex';
import DarkBaseTheme from '../base-themes/dark-base-theme';

export default {
Copy link
Member

Choose a reason for hiding this comment

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

how about export default DarkBaseTheme;?

@alitaheri alitaheri added the PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI label Dec 16, 2015
@alitaheri alitaheri added this to the 0.14.0 Release milestone Dec 16, 2015
@alitaheri
Copy link
Member

@mbrookes nice work on the rename 👍 👍 Thanks ^_^

@alitaheri
Copy link
Member

Also I think we'll need major rename across the documentation too. -_-

@oliviertassinari
Copy link
Member

I'm wondering, why are we uppercasing the first lettre of the theme files name. For instance LightBaseTheme.
We are not dealing with classes but simple objects.

@mbrookes
Copy link
Member Author

Thanks @newoga, didn't know about deprecatedExport, but it looks like it's specific to components. When it comes to deprecation I'll take a look at warning, and can reuse the wording in deprecatedExport.

@subjectix export default LightBaseTheme; That makes much more sense, thanks!

Yes, docs are the main place where the raw-theme naming is exposed, so will definitely need to be updated. Wanted to get a consensus on the approach before undertaking that change.

@oliviertassinari - good question. I did wonder, but just followed current convention.

@mbrookes
Copy link
Member Author

@newoga - there is one option here:

import LightBaseTheme from '../base-themes/light-base-theme';
import deprecatedExport from '../../utils/deprecatedExport';

LightBaseTheme.displayName = 'LightRawTheme';

export default deprecatedExport(
  LightBaseTheme,
  'material-ui/lib/styles/raw-themes/light-raw-theme',
  'material-ui/lib/styles/base-themes/light-base-theme'
);

But the resulting error message is a little confusing:

"Warning: Importing LightRawTheme from 'material-ui/lib/styles/raw-themes/light-raw-theme' has been deprecated, use 'material-ui/lib/styles/base-themes/light-base-theme' instead."

Is it 'LightRawTheme' or 'LightBaseTheme' at that point?

@alitaheri
Copy link
Member

@mbrookes That's one hell of a question :D :D Well I think we can be a little aggressive on this and call them LightBaseTheme. I think the confusion is a good encouragement to change the name.

@newoga
Copy link
Contributor

newoga commented Dec 16, 2015

@mbrookes @subjectix Haha! I agree, that's a very philosophical question 😆. I agree with @subjectix, if I had to pick between the two, LightBaseTheme might be better.

Though @mbrookes has a good point, when we created this, it was originally designed for components. I suppose the easiest thing we could do is to modify the behavior of the deprecatedExport function when the exported object doesn't have a displayName property.

For example:

// Component.displayName = 'Component': 
export default deprecatedExport(
  Component,
  'material-ui/lib/component',
  'material-ui/lib/Component'
);
/* Warning: Importing Component from 'material-ui/lib/component' has been deprecated, use 'material-ui/lib/Component' instead.

And with no displayName

//UtilityObject.displayName is undefined: 
export default deprecatedExport(
  UtilityObject,
  'material-ui/lib/utility-object',
  'material-ui/lib/UtilityObject'
);
/* Warning: Importing from 'material-ui/lib/utility-object' has been deprecated, use 'material-ui/lib/UtilityObject' instead.

@newoga
Copy link
Contributor

newoga commented Dec 16, 2015

Also, maybe we should call the files LightBaseTheme.js and DarkbaseTheme.js instead of light-base-theme.js and dark-base-theme.js. I figured we might as well do it here since we've started migrating some of the components like TextField.jsx.

@oliviertassinari @subjectix Thoughts?

@alitaheri
Copy link
Member

@newoga I like the intuitive naming, but can we please drop the capitals from all over the place where the export is not a component ? -_- @oliviertassinari would agree :D

@mbrookes
Copy link
Member Author

So "lightBaseTheme.js" for the file name, and lightBaseTheme for the variable? That was my next question! :)

@newoga - please could you adapt deprecatedExport when you get a chance, and I'll use it here.

@oliviertassinari
Copy link
Member

@subjectix Yes, I agree with you 👯.

@newoga
Copy link
Contributor

newoga commented Dec 16, 2015

@subjectix I agree on making non-components camelCase instead of PascalCase! Sounds good to me! One day everything is going to be named perfectly, then we can celebrate 🍻

@mbrookes I'll get that deprecatedExport fix in right away and report back

@alitaheri
Copy link
Member

Thanks everyone for your feedbacks and hard work 👍 👍

@newoga
Copy link
Contributor

newoga commented Dec 17, 2015

@oliviertassinari @subjectix @mbrookes The changes to deprecatedExport are at #2564

@mbrookes
Copy link
Member Author

Updated to camelCase and to use #2564.

Todo:

  • Update styles/index.js to use baseThemes for rawTheme exports
  • Update components to use lightBaseTheme as DefaultRawTheme
  • Update 'customization' docs

@alitaheri
Copy link
Member

@mbrookes don't touch the components yet. wait for them to use the theme hoc

we'll do a whole re run at the latest stages before the 0.14.0 release

@mbrookes
Copy link
Member Author

@subjectix 👌

@newoga
Copy link
Contributor

newoga commented Dec 17, 2015

@mbrookes Could you add to the todo list to modify styles/theme-manager.js to include the baseTheme at key of baseTheme, not just rawTheme?

If that change is done, components that reference rawTheme from a theme in context can safely switch to baseTheme.

I wouldn't be opposed to you implementing that change now as long as we leave rawTheme in there as well.

@alitaheri
Copy link
Member

@mbrookes would it be ok with you if I base my work on your PR? I want to try to implement some theme related stuff, but these changes will conflict like hell :D so I'll base it on your work if that's ok 😁

@mbrookes
Copy link
Member Author

Cool with me. Sorry I haven't caught up with the latest suggestions, but feel free to take this wherever you see fit - I can easily merge / rebase.

@alitaheri
Copy link
Member

@mbrookes Don't touch this PR anymore. I rebased, modified and updated your work. And based other works opon it 😁 Thanks for the help 👍 👍

@alitaheri
Copy link
Member

I continued the work here in #2585 I'll close this PR. Please continue any further discussion there

@alitaheri alitaheri closed this Dec 18, 2015
@mbrookes mbrookes deleted the base-themes branch March 19, 2016 01:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants