-
-
Notifications
You must be signed in to change notification settings - Fork 32.1k
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
[Core] Initial style refactor #2907
Conversation
What are your thoughts on removing With this PR, the only place And of course, we can also deprecate immutability-helper and remove all uses of it internally until |
Also, let me know your thoughts on renaming That way we have: /* Old mixin behavior */
this.mergeStyles()
this.prepareStyles()
/* util */
styleUtils.merge()
styleUtils.prepare() I suppose a third approach is to leave the method names the same (change merge back to mergeStyles), then you could do something like this: import {mergeStyles, prepareStyles} from '../utils/styles'
/**
* Also works like ...
* import * as styleUtils from '../utils/styles';
* styleUtils.mergeStyles();
* styleUtils.prepare()
*/
const SomeComponent = ({muiTheme, style}) => {
const rootStyle = {};
return (
<div style={prepareStyles(muiTheme, mergeStyles(rootStyle, style)}/>
);
}; To be fair, fourth approach is just to abandon merge all together and use destructuring like @yongxu mentioned. import {prepareStyles} from '../utils/styles'
const SomeComponent = ({muiTheme, style}) => {
const rootStyle = {};
return (
<div style={prepareStyles(muiTheme, {...rootStyle, ...style}}/>
);
}; Sorry for dropping all of this, but we'll soon be ready for removing style-propable and just want to make sure we agree on a syntax that we're happy with. |
Thanks a lot @newoga, This is a big help 👍
I completely agree! It's not that useful and it's very confusing! I mean, there are many libraries to handle such things. why do we have our own.... oh wait it WRAPS another library :| I think merge must be removed 😁 and prepareStyles cannot be confused with prepare. I think prepare is easier to write without confusion. |
That sounds like a good idea. It's only used in the Regarding the syntax we can use. I really like this one: import {mergeStyles, prepareStyles} from '../utils/styles'
const SomeComponent = ({muiTheme, style}) => {
const rootStyle = {};
return (
<div style={prepareStyles(muiTheme, mergeStyles(rootStyle, style)}/>
);
}; |
Thanks for your feedback guys. I made a few additional commits that I would like your feedback before continuing. First, @alitaheri @oliviertassinari After experimenting a bit, I decided that this syntax is the best route. import {mergeStyles, prepareStyles} from '../utils/styles'
const SomeComponent = ({muiTheme, style}) => {
const rootStyle = {};
return (
<div style={prepareStyles(muiTheme, mergeStyles(rootStyle, style)}/>
);
}; I put details as to why in this commit message. But ultimately I realized that this pattern better follows the functional approach we are heading in as well as offers bunch of benefits as it relates to our eslinter (such as detecting unused methods and correct method names). I noticed it immediately when I made the change. Also, |
FYI, whether we should continue using our
Sorry, I forgot to address this. Does this mean that you think we should switch everything to destructuring? I don't disagree, I just don't know if we need to confirm performance implications. Let me know! |
Thank you for your awesome work guys! This looks great 👍 |
@newoga Nice work. I'm really happy with it 👍. Could you squash down the commit? |
@newoga I didn't mean we must use destructuring. I was only saying that we should leave such trivial things to external libraries like lodash (babel's polyfill). Since this is what they are really good at. For now it's all good. it doesn't matter that much anyway. this is very good for now. thanks 👍 👍 👍 @oliviertassinari Yes I'm all good |
This commit includes a number of changes: 1. Removes all apply() calls in favor of argument destructuring 2. Simplifies `style-propable` mixin to import/wrap solely from `utils/styles` 3. Search the code base for components that used `utils/immutability-helper` and replaced it with `utils/styles/mergeStyles` or `react-addons-update` 4. Removed `utils/immutability-helper` from src 5. Renamed `utils/styles` merge() to mergeStyles() 6. Prepared deprecation of passing more than one style to `prepareStyles()` (it is imlemented but commented) 7. Fixed all components that are dependent on `utils/styles` to follow new convention (do not import `utils/styles` default export to a `styleUtils` object, just import the method you need) Signed-off-by: Neil Gabbadon <neil.gabbadon@emikra.com>
d758702
to
5ec947f
Compare
@alitaheri Okay right, I agree. Thanks for clarifying. 😄 There are still a few remaining pieces I want to solve, we can revisit in next PR. @oliviertassinari @alitaheri In case you need to review anything else before merging, I made a backup of the branch before rebasing in case you need to see the separate commits. Thanks guys, if all is green, feel free to merge. |
It's all good @newoga Thanks a lot for your hard work on this 👍 👍 👍 @oliviertassinari I'll leave the merge to you. All green here 💚 |
[Core] Initial style refactor
@oliviertassinari @alitaheri
This is the start of some of the refactors that I wanted to do, but there is still more. So far I've done the following:
Simplifies style utilities and removes utils/immutability-helper.js
This commit includes a number of changes:
style-propable
mixin to import/wrap solely fromutils/styles
utils/immutability-helper
and replaced it withutils/styles/mergeStyles
orreact-addons-update
utils/immutability-helper
from srcutils/styles
merge() to mergeStyles()prepareStyles()
(it is imlemented but commented)utils/styles
to follow new convention (do not importutils/styles
default export to astyleUtils
object, just import the method you need)Todo:
ensureDirection
mergeStyles
or use spread operators in components