[Theme] Fix a few unthemable elements #2892

pdf · 2016-01-12T10:11:38Z

Used palette.textColor to replace hard-coded blacks, should work for
both dark and light themes

Closes #3135.

oliviertassinari · 2016-01-12T19:13:28Z

@pdf Thanks for this PR, but let us fix #2901 before reviewing it.

pdf · 2016-01-13T03:36:11Z

@oliviertassinari sounds like a fair plan. FYI, I found a bunch more hard-coded values, I think I've got nearly all of them, except for one or two like error colors and media overlays. Should be pretty close to having themes apply cleanly now, though there may still be some places where the default body text color was being assumed.

I did also change the ColorManipulator API a little, exposing luminance and adding an opacity param to lighten (on that note, any idea why lighten forces opacity to 0.15 by default?). The changes there should be backwards-compatible.

pdf · 2016-01-15T11:18:41Z

Rebased against master

oliviertassinari · 2016-01-21T13:50:09Z

src/avatar.jsx

@@ -86,6 +83,10 @@ const Avatar = React.createClass({
    this.setState({muiTheme: newMuiTheme});
  },

+  getTheme() {


Can we remove the usage of getTheme method from this PR? It's always called multiple times inside the same function. That seems inefficien.

I went with this logic because it is used already in multiple components...

I went with this logic because it is used already in multiple components...

That's true, we should be consistent regarding this point, but that's not the case right now in our code base.
@alitaheri @newoga What do you think?

Yeah I think we should stop using the function. I haven't see the getTheme function pattern around in the library much but if it's around, we should move towards not doing it.

Personally, even if we need a function to help with codeflow/logic I'd rather put the function at the top of the module rather as a member of the React component. The more functions we have of components, the more likely someone will read source and abuse them.

Actually, this is one of the reasons why I prefer using functional components when possible over classes because it just takes the choice away.

In addition, this particular issue is why I was hoping we could figure out something like #2509. A lot of times when we do have the theme, we do have to traverse down a lot of nested properties to get what we need. I'm still not sure best approach but I'm not entirely convinced our current one is the best one.

I agree with the removal of getTheme it seems really redundant.

we do have to traverse down a lot of nested properties to get what we need

It's not us it's javascript 😆 😆 although nested destructuring takes some of the pain away.

It's not us it's javascript 😆 😆 although nested destructuring takes some of the pain away

Haha! I agree about regarding the nested destructuring

@pdf @oliviertassinari Let's get rid of the getTheme function, looking again this is fairly simple component, and I think it's fine just breaking it up earlier into a const in the render method. Ironically this component could become a functional component once style-propable is gone.

newoga · 2016-01-22T19:17:05Z

@pdf Thanks for your work on this!

pdf · 2016-01-23T03:25:52Z

I believe this commit addresses feedback. It looks like the list of components below currently use the getTheme() pattern, should they be updated for consistency? There's also inconsistency in the use of a getStyle() property across various components. I'd be tempted to look at these things in a separate PR though.

grep -Irl getTheme() src/:

src/linear-progress.jsx
src/toolbar/toolbar-title.jsx
src/toolbar/toolbar-separator.jsx
src/toolbar/toolbar.jsx
src/slider.jsx
src/raised-button.jsx
src/radio-button.jsx
src/table/table-row.jsx
src/table/table-header.jsx
src/table/table-header-column.jsx
src/table/table.jsx
src/table/table-footer.jsx
src/table/table-row-column.jsx
src/time-picker/time-display.jsx
src/time-picker/clock-pointer.jsx
src/time-picker/clock-number.jsx
src/time-picker/time-picker-dialog.jsx
src/menu/menu.jsx
src/menu/menu-item.jsx
src/menu/link-menu-item.jsx
src/menu/subheader-menu-item.jsx
src/enhanced-switch.jsx
src/checkbox.jsx
src/circular-progress.jsx
src/floating-action-button.jsx
src/refresh-indicator.jsx
src/toggle.jsx
src/date-picker/year-button.jsx
src/date-picker/date-display.jsx
src/date-picker/day-button.jsx

oliviertassinari · 2016-01-23T13:34:31Z

src/lists/list.jsx

@@ -101,7 +103,7 @@ const List = React.createClass({
      },

      subheader: {
-        color: Typography.textLightBlack,
+        color: theme.subheaderTextColor,


Will conflict with https://github.com/callemall/material-ui/pull/2979/files#diff-509c50933d8b7384457d8c9e34a34374L104.
I feel like it would be better to merge the other PR that introduce a Subheader component, then this one.

oliviertassinari · 2016-01-23T13:41:09Z

It looks like the list of components below currently use the getTheme() pattern, should they be updated for consistency?

Yes, we should do it at some point but I think that it's out of the scope of this PR.

pdf · 2016-01-24T03:24:58Z

Actioned feedback, rebased against master. Will update again if you want to merge #2979 first.

oliviertassinari · 2016-02-03T17:53:29Z

@pdf The Subheader PR was merged. Could you rebase this PR? It would be great to merge it 👍. Thanks.

chris-hinds · 2016-02-04T14:39:02Z

Any info on when this will be merged? I could really do with the card title and headers being themeable.

I have tried making the changes manually but to no avail.

pdf · 2016-02-04T14:40:49Z

I should be able to take another pass at this on the weekend.

chris-hinds · 2016-02-04T14:43:09Z

That would be fantastic, Like I said tried making the changes I can see here by adding the theme colours but that didn't seem to take affect at all. the card background colour changes just not the titles of the subtitles :/

pdf · 2016-02-06T01:07:46Z

Well, 334a1a8 and some others make this rebase pretty laborious. I think it might be easier to rewrite at this stage, will take me a little bit.

Replace hard-coded color values with colors derived from either text or canvas color. Closes mui#3135

pdf · 2016-02-06T02:14:00Z

Had to bump eslint-plugin-react to get the docs to build, think this should be pretty close. There might still be instances of body text color being assumed to be black for the light theme (which can result in illegible text for dark themes), but I think fixing that for AppCanvas and Paper should cover most cases.

alitaheri · 2016-02-06T06:45:44Z

@pdf This is great! Thanks a lot 👍 👍

I'm all green @oliviertassinari take a look and merge 😁

chris-hinds · 2016-02-08T10:16:37Z

Eagerly awaiting this merge :D thanks for the hardwork @pdf and @alitaheri

alitaheri · 2016-02-08T10:48:29Z

Seems like there is a conflict here. @pdf Could you take a look?

pdf · 2016-02-08T20:22:41Z

There's a conflict now because a whole truckload of PRs were just merged in front of this again, including stuff like a cleanup of getStyles that I mentioned earlier in this very discussion. Because this PR touches a lot of files, every time this happens, it makes for a painful rebase. I don't have time to keep fixing this right now.

oliviertassinari · 2016-02-08T21:24:38Z

I don't have time to keep fixing this right now

Sorry about it! And the migration processes isn't finished yet. We can wait until it's done 😁.

pdf · 2016-02-08T22:19:53Z

Judging by the changes, it'll probably be another re-write for this PR if there're more components to be updated in similar fashion. I guess just ping me at some stage when you think there's a good time to get this merged. This is for a personal project though, so I can only work on it in my spare time.

oliviertassinari · 2016-02-08T22:29:42Z

I guess just ping me at some stage when you think there's a good time to get this merged.

Thanks, I will keep you up to date!

This is for a personal project though, so I can only work on it in my spare time.

Same as me 👍

newoga · 2016-02-08T22:40:39Z

@pdf Would it be alright if I gave you a list of components that are already migrated, then you could start with those?

I really appreciate your patience and work on this.

newoga · 2016-02-08T22:43:40Z

Actually on second look the only component you modified that hasn't been migrated yet is <FloatingActionButton />, I can finish that now then you should be safe to fix this.

pdf · 2016-02-08T22:48:21Z

@newoga rebase conflicts will tell me which components have been updated 😉, but I won't be able to get back to this for a few days at a minimum (possibly a couple of weeks as I have some other engagements coming up), so don't rush anything on my account.

newoga · 2016-02-08T22:49:06Z

Okay no worries then! Thanks much @pdf!

alitaheri · 2016-02-09T18:32:33Z

@pdf This requires a lot of work! I'm intercepting this PR. Don't work on it anymore, I'll fix the conflicts tonight so we can merge this soon 👍 Thanks a lot for your initial work on this 👍

alitaheri · 2016-02-09T19:17:14Z

Being continued #3269.

Thanks @pdf For your hard work on this. You seem busy, so it'll take it from here 👍 👍 😁

pdf force-pushed the unthemable_elements branch 2 times, most recently from 511ffaf to b05418b Compare January 12, 2016 10:31

oliviertassinari mentioned this pull request Jan 12, 2016

[Docs] Allow to switch the theme on the all documentation #2901

Closed

oliviertassinari added the PR: Needs Review label Jan 12, 2016

pdf force-pushed the unthemable_elements branch from b05418b to 3082089 Compare January 13, 2016 03:28

pdf force-pushed the unthemable_elements branch from 3082089 to b657650 Compare January 15, 2016 11:13

oliviertassinari changed the title ~~Fix a few unthemable elements~~ [Theme] Fix a few unthemable elements Jan 15, 2016

oliviertassinari reviewed Jan 21, 2016
View reviewed changes

oliviertassinari added PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI and removed PR: Needs Review labels Jan 21, 2016

pdf force-pushed the unthemable_elements branch 2 times, most recently from 2a4b822 to 50a74a9 Compare January 23, 2016 03:20

oliviertassinari reviewed Jan 23, 2016
View reviewed changes

pdf force-pushed the unthemable_elements branch from 50a74a9 to 04acee8 Compare January 24, 2016 03:19

oliviertassinari mentioned this pull request Feb 2, 2016

[Card] Title and Subtitle color not affected by dark theme. #3135

Closed

Bump eslint-plugin-react

2867e20

pdf force-pushed the unthemable_elements branch from 04acee8 to 69335df Compare February 6, 2016 02:00

Fix a number of unthemeable elements.

c402885

Replace hard-coded color values with colors derived from either text or canvas color. Closes mui#3135

pdf force-pushed the unthemable_elements branch from 69335df to c402885 Compare February 6, 2016 02:12

alitaheri mentioned this pull request Feb 6, 2016

[Card] Remove style-propable mixin #3198

Merged

alitaheri mentioned this pull request Feb 9, 2016

Unthemable elements #3269

Merged

alitaheri closed this Feb 9, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Theme] Fix a few unthemable elements #2892

[Theme] Fix a few unthemable elements #2892

pdf commented Jan 12, 2016

oliviertassinari commented Jan 12, 2016

pdf commented Jan 13, 2016

pdf commented Jan 15, 2016

oliviertassinari Jan 21, 2016

pdf Jan 21, 2016

oliviertassinari Jan 21, 2016

newoga Jan 22, 2016

alitaheri Jan 22, 2016

newoga Jan 22, 2016

newoga commented Jan 22, 2016

pdf commented Jan 23, 2016

oliviertassinari Jan 23, 2016

oliviertassinari commented Jan 23, 2016

pdf commented Jan 24, 2016

oliviertassinari commented Feb 3, 2016

chris-hinds commented Feb 4, 2016

pdf commented Feb 4, 2016

chris-hinds commented Feb 4, 2016

pdf commented Feb 6, 2016

pdf commented Feb 6, 2016

alitaheri commented Feb 6, 2016

chris-hinds commented Feb 8, 2016

alitaheri commented Feb 8, 2016

pdf commented Feb 8, 2016

oliviertassinari commented Feb 8, 2016

pdf commented Feb 8, 2016

oliviertassinari commented Feb 8, 2016

newoga commented Feb 8, 2016

newoga commented Feb 8, 2016

pdf commented Feb 8, 2016

newoga commented Feb 8, 2016

alitaheri commented Feb 9, 2016

alitaheri commented Feb 9, 2016

[Theme] Fix a few unthemable elements #2892

[Theme] Fix a few unthemable elements #2892

Conversation

pdf commented Jan 12, 2016

oliviertassinari commented Jan 12, 2016

pdf commented Jan 13, 2016

pdf commented Jan 15, 2016

oliviertassinari Jan 21, 2016

Choose a reason for hiding this comment

pdf Jan 21, 2016

Choose a reason for hiding this comment

oliviertassinari Jan 21, 2016

Choose a reason for hiding this comment

newoga Jan 22, 2016

Choose a reason for hiding this comment

alitaheri Jan 22, 2016

Choose a reason for hiding this comment

newoga Jan 22, 2016

Choose a reason for hiding this comment

newoga commented Jan 22, 2016

pdf commented Jan 23, 2016

oliviertassinari Jan 23, 2016

Choose a reason for hiding this comment

oliviertassinari commented Jan 23, 2016

pdf commented Jan 24, 2016

oliviertassinari commented Feb 3, 2016

chris-hinds commented Feb 4, 2016

pdf commented Feb 4, 2016

chris-hinds commented Feb 4, 2016

pdf commented Feb 6, 2016

pdf commented Feb 6, 2016

alitaheri commented Feb 6, 2016

chris-hinds commented Feb 8, 2016

alitaheri commented Feb 8, 2016

pdf commented Feb 8, 2016

oliviertassinari commented Feb 8, 2016

pdf commented Feb 8, 2016

oliviertassinari commented Feb 8, 2016

newoga commented Feb 8, 2016

newoga commented Feb 8, 2016

pdf commented Feb 8, 2016

newoga commented Feb 8, 2016

alitaheri commented Feb 9, 2016

alitaheri commented Feb 9, 2016