[Divider] Initial implementation #2473
Conversation
|
Side thought, I noticed we have
I think components that aren't using the mixin should be switched to use the style util class instead of importing the immutability helper. |
|
@newoga In the es6-classes branch we are trying to migrate the whole project to classes and remove mixins. So, if you wish to help us address that too take a look at that branch. Thanks for doing this. I'll do a review asap 👍 👍 |
newoga
changed the title
|
Sure, I'll take a look at the Thanks @subjectix, and no rush on reviewing these pull requests. I know you guys are busy getting a lot of the other big pieces in place like docs, es6, etc. |
|
I got 113 errors over 10 seconds in the List's Doc page O.O Could you please migrate the docs too? I like it overall and we've agreed to do this. so, no effort wasted 😁 |
| function ListDivider(props) { | ||
| warning(false, '<ListDivider /> has been deprecated. Please use the <Divider /> component.'); | ||
| return <Divider {...props} />; | ||
| } |
There was a problem hiding this comment.
this warning goes in the render function of this component, please use createClass and put this in the getInitialState so that it happens only once per instantiation rather than per render. thanks ^^
|
Thanks a lot 👍 I'm ok with this 🎉 @oliviertassinari Please review 😁 |
alitaheri
changed the title
| import DefaultRawTheme from '../styles/raw-themes/light-raw-theme'; | ||
| import ThemeManager from '../styles/theme-manager'; | ||
| import Divider from '../divider'; | ||
| import Styles from '../utils/styles'; |
There was a problem hiding this comment.
I think that it should be in lower case. It's an utility object, not a class.
Maybe, we could call it utilsStyles.
There was a problem hiding this comment.
I think we should do a whole rewrite with these, I'm still wrapping my head around how styles are actually handled -_-
There was a problem hiding this comment.
I think we should do a whole rewrite with these
Alright, for later. But since we are not using the most used way (with a mixin), I though It was the right time to do it right.
There was a problem hiding this comment.
I don't mind renaming, I initially called it Styles because that's the name it was imported under in style-propable.js. But I understand that is going to be removed soon.
@oliviertassinari , @subjectix ,
If we were to rename this, what do you both prefer?
utilsStyles
styleUtil
styleUtils
alitaheri
added
PR: needs revision
|
@subjectix , @oliviertassinari I noticed though in one of my doc examples the Menu component wasn't properly using its supplied width prop. Can't figure out why, maybe I'm doing something wrong... Aside from that, I think I'm all wrapped up with this unless you want any other changes. Thanks for all the input! |
alitaheri
added
PR: Review Accepted
and removed
PR: needs revision
|
@newoga All's good, A job very well done 👍 👍 There is a tiny problem with menu in the docs, but that's the menu's fault. We'll come back to that later. thanks a lot for this 🎉 👍 if @oliviertassinari Gives the go, do a rebase and I'll merge. |
| const DividerExampleForm = () => { | ||
| return ( | ||
| <Paper zDepth={2}> | ||
| <TextField hintText="First name" underlineStyle={{display: 'none'}} style={{marginLeft: 20}} /> |
There was a problem hiding this comment.
What about having a small style variable to share this across component?
There was a problem hiding this comment.
True, no reason to be lazy now! That being said, after we merge #2476 , I was planning on coming back to this example to use to use that underlineShow prop instead of the underlineStyle. Would still need a variable like you said something for the left margin.
If we #2476 is close and we can merge first, I can fix this once and for all.
|
@newoga It's almost perfect 👍 ! Could you have a final view at my comments? |
|
|
||
| import MobileTearSheet from '../mobile-tear-sheet'; | ||
|
|
||
| const DividerExampleList = () => ( |
There was a problem hiding this comment.
Also, let me know know if you're okay with this or if you want me to switch it back...
Since this component isn't doing anything with props I changed it like this:
/* BEFORE */
const Component = () => {
return (
<div>
<p>Hello world!</p>
</div>
);
}
/* After */
const Component = () => (
<div>
<p>Hello world!</p>
</div>
);There was a problem hiding this comment.
For me it's fine. @subjectix?
There was a problem hiding this comment.
It's alright for the examples, but don't follow it in the code. or we'll have ugly diffs 😁
There was a problem hiding this comment.
Sounds good! I'll make sure not to do it in the source code 😄
|
@oliviertassinari, I just rewrote and pushed a stateless functional implementation of the Divider and it appears to work. Can you take a look? |
| inset: false, | ||
| }; | ||
|
|
||
| const Divider = ({inset, style, ...other}, {muiTheme = ThemeManager.getMuiTheme(DefaultRawTheme)}) => { |
There was a problem hiding this comment.
What if the context came from a parent component via the context?
We were using
muiTheme: this.context.muiTheme ? this.context.muiTheme : ThemeManager.getMuiTheme(DefaultRawTheme),There was a problem hiding this comment.
I think essentially this:
{muiTheme = ThemeManager.getMuiTheme(DefaultRawTheme)}Does the same thing. I think if muiTheme is in context, it will use it. If it is undefined, it will default to ThemeManager.getMuiTheme(DefaultRawTheme).
I tested the code without the = ThemeManager.getMuiTheme(DefaultRawTheme) and it still works.
I'll be honest, I'd love to tell you that I know for sure this is the same exact behavior as what we were doing before, but I'm not sure. But based on the docs I'm reading and what I'm seeing with the code, it looks like it's working.
Edit: The = in the destructuring syntax allows us to set a default when it is undefined.
There was a problem hiding this comment.
I can revert this latest commit and we can explore this stateless with context approach separately if you want. This context stuff is still a little new to me. material-ui is the first time I'm really running into it.
There was a problem hiding this comment.
The = in the destructuring syntax allows us to set a default when it is undefined.
Alright, sounds fair.
The code seems really nice. I think that you can rebase, 👍
I will checkout your change on my local to test it before merging.
|
@oliviertassinari , rebased and pushed. Let me know how it goes, thanks for the feedback! |
|
@newoga Awesome work! I'm 👍, merge this @oliviertassinari If you're good with it. |
| @@ -0,0 +1,5 @@ | |||
| ## Divider | |||
|
|
|||
| Dividers group and separate content within lists and page layouts. The divider is a thin rule, lightweight yet sufficient to distinguish content visually and spatially. | |||
There was a problem hiding this comment.
Could you attach a link to https://www.google.com/design/spec/components/dividers.html to the Dividers word?
|
@newoga It's almost ready to be merged 🎉. |
|
Great! Thanks @oliviertassinari, I can squash back down to one commit if you want. |
|
@newoga Do a squash and I'll merge 👍 🎉 |
| @@ -0,0 +1,5 @@ | |||
| ## Divider | |||
|
|
|||
| <a href="https://www.google.com/design/spec/components/dividers.html" target="_blank">Dividers</a> group and separate content within lists and page layouts. The divider is a thin rule, lightweight yet sufficient to distinguish content visually and spatially. | |||
There was a problem hiding this comment.
What about [Dividers](https://www.google.com/design/spec/components/dividers.html) instead?
It would be better to not be linked to the dom to use https://github.com/lwansbrough/react-native-markdown later.
There was a problem hiding this comment.
oh, I missed that, good catch.
There was a problem hiding this comment.
Yeah, I initally made a normal link, but I thought it'd be better to open the spec in a window/tab.
Is there a way to do that with MD?
If not, I can change anyway...
There was a problem hiding this comment.
[Dividers](https://www.google.com/design/spec/components/dividers.html){:target="_blank"}?
There was a problem hiding this comment.
Well It's not working 😬. We have to choose what's more important.
I would go for not using the dom. What do you think guys?
There was a problem hiding this comment.
Mm, no luck with that. Maybe it's alright if it just opens in the same window?
It kind of threw me off the first time I tried it but maybe it's fine.
There was a problem hiding this comment.
Did a new push with [Dividers](https://www.google.com/design/spec/components/dividers.html)
|
@newoga I think that we have raised a lot of questions along this PR. That was really valuable 👍. |
[Divider] Initial implementation
|
@newoga Thanks a lot for doing this, A job well done 🎉 👍 👍 |
|
Thanks so much @oliviertassinari @subjectix , cheers to everyone 🎉 👍 ! |
zannager
added
the
component: divider
This resolves #2457.
Done:
divider.jsxin root with same implementation as originallist-divider.jsxlist-divider.jsxandmenu-divider.jsxas stateless components that wrapdivider.jsxand emit warnings when used. (menu-divider.jsxapplies original style overrides for backwards compatibility).list-divider.jsxandmenu-divider.jsxin internal components and documentation.<Divider />component when used independently of other components.Todo: