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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[AppBar] Remove style-propable mixin #3183

Merged
merged 1 commit into from
Feb 5, 2016
Merged

Conversation

newoga
Copy link
Contributor

@newoga newoga commented Feb 5, 2016

Signed-off-by: Neil Gabbadon neil.gabbadon@emikra.com

@alitaheri
Copy link
Member

@newoga I have a proposal for these things:

What if we do this like:

// imports ...

const styles = {
  mainElement: { // this will always be the same instance forever!
    boxFlex: 1,
    flex: '1',
  },
  root: (someProp, otherProp) => ({ // this can be memoized later on with ease
    // calculate according to only two props, only recalculate if any of the props change
  }),
  title: (theme) => ({ // this can be initiated once per theme.
    // calculate according to theme, only recalculate if theme changes.
  }),
  // ...
};

// the component

This is only for discussion. if you guys think this is worth a discussion I'll open an issue for it.

@newoga @oliviertassinari @mbrookes

marginRight: 8,
marginLeft: -16,
},
iconStyle: {
Copy link
Member

Choose a reason for hiding this comment

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

What about flattening style and iconStyle?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I would prefer that too 👍 I'll give it a go, thanks!

@newoga
Copy link
Contributor Author

newoga commented Feb 5, 2016

This is only for discussion. if you guys think this is worth a discussion I'll open an issue for it.

Sure, open an issue up 😁! I also was wondering if an approach like that was possible but I hadn't fully conceptualized and tried anything yet. 👍

Signed-off-by: Neil Gabbadon <neil.gabbadon@emikra.com>
@newoga
Copy link
Contributor Author

newoga commented Feb 5, 2016

Done, flattened iconButton.style and iconButton.iconStyle to root level, and switch setState to nextContext.muiTheme || this.state.muiTheme.

@oliviertassinari
Copy link
Member

That looks good. @alitaheri Feel free to merge 👍

alitaheri added a commit that referenced this pull request Feb 5, 2016
[AppBar] Remove style-propable mixin
@alitaheri alitaheri merged commit 087b95f into mui:master Feb 5, 2016
@alitaheri
Copy link
Member

Thanks a lot, this is looking good 👍 😍

@alitaheri
Copy link
Member

Sure, open an issue up 😁! I also was wondering if an approach like that was possible but I hadn't fully conceptualized and tried anything yet. 👍

@newoga Actually I rather investigate further. it's not so straight forward. Since we should also support SSR, we'll have some issues we have to handle properly. so I might even build my own package for this. It's gonna need a bit work and thought. I'll get back to you guys when I think of something 😄

@oliviertassinari
Copy link
Member

@alitaheri That's an interesting proposal and doesn't seem to be linked to the style approach we follow. For instance I think that it would work with css classname. Well, in this case it would be overkill 😁.

@alitaheri
Copy link
Member

@oliviertassinari No i won't go for what I purposed up there. I'm thinking more of a HOC that passes down styles if they change. this means the calculated styles will only be passed down if they change and if they don't change the render will stop. if they change only the changed styles will be new instances not every one of them. I still haven't figured out how to achieve this. But i think this is necessary. I'll probably create a repository for it with lots of tests for it before getting back to you.

@oliviertassinari
Copy link
Member

@alitaheri I love this idea. That would make our component unstyled by default 😍.

@alitaheri
Copy link
Member

Thanks ^_^ I'm gonna think of something soon, stay tuned 😆 😆

@newoga
Copy link
Contributor Author

newoga commented Feb 5, 2016

@alitaheri Good luck with that! I had a very similar idea when I thought of moving the getStyles() function out. I look forward to see what you come up with 😁 By the way, maybe you could use https://github.com/rackt/reselect as an inspiration. My recent work with that lib made me wonder if we should move toward this approach 😁

@alitaheri
Copy link
Member

@newoga I actually use reselect in my application. That's where I understood what memoization what 😁 So I'll use that as a reference for sure 👍

@newoga newoga deleted the #2852/app-bar branch February 6, 2016 23:25
@zannager zannager added the component: app bar This is the name of the generic UI component, not the React module! label Mar 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: app bar This is the name of the generic UI component, not the React module!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants