[Docs] [DropDownMenu] [Composable] Migrate to new standard #2565
Conversation
| ## Drop Down Menu | ||
|
|
||
| To find out more about the `DropDownMenu` component please visit the Material Design's | ||
| specifications **WHERE IS IT IN THE SPEC?!**. |
There was a problem hiding this comment.
@oliviertassinari What is the reference? I couldn't find it 😁
There was a problem hiding this comment.
There isn't one I don't think, just the second-last animation here: https://www.google.com/design/spec/components/menus.html#menus-usage (Textfield dropdown).
There was a problem hiding this comment.
Yup, I agree with @mbrookes. Unfortunately the spec isn't very clear on this type of component and a lot has to be inferred based on screenshots from the menu and text field sections. I wish the spec was more explicit with some of the controls 😞
There was a problem hiding this comment.
Indeed, the spec isn't breaking thing too much into component.
But I'm wondering, what's the difference between the DropDownMenu and the SelectField?
I may be wrong, but I feel like the SelectField is enought.
There was a problem hiding this comment.
@oliviertassinari Yes I've wondered the same thing since I started using this library. In fact, the only think select field does is wrap DropDownMenu. We'll have to address this after #2555 in my opinion
There was a problem hiding this comment.
You are right, #2416 makes a lot of sense 👍.
Let's wait 0.14 before taking care of it.
There was a problem hiding this comment.
@mbrookes I'm gonna use the link you gave. thanks 👍
|
@subjectix I gave it a quick review and I like what's been done so far 👍 . I'm sure you have more changes in stow so I'll leave a few comments on the lines of code and keep tuned! |
| }, | ||
|
|
||
| _setSelectedIndex(index) { | ||
| if (process.env.NODE_ENV !== 'production') { |
There was a problem hiding this comment.
I think we can remove this production check since the warning package does it for us.
|
Still a work in progress. I patched up the feedbacks. Ignore all the deprecation warnings in the docs site. I'm working on a lot of things 😬 |
|
@oliviertassinari Are you ok with the descriptions? |
|
@subjectix Yes, I'm. |
|
@oliviertassinari Possibly because your screen can't fit it. Please see if this fixes it |
|
@subjectix It's ok now 👍 |
|
@oliviertassinari Thanks for the feedback 👍 😄 |
alitaheri
changed the title
|
This is final. @oliviertassinari @newoga @mbrookes Please take a final look, before I squash and merge ^_^ |
alitaheri
force-pushed
the
composable-drop-down-menu
branch
from
cb79abe
to
a83dbee
Compare
|
Sorry had to squash, for the merge conflict It was a pain rebasing them to this clean history -_- |
| * **DEPRECATED** `DropDownMenu` will use this member to display | ||
| * the name of the item on the label. | ||
| */ | ||
| labelMember: React.PropTypes.string, |
There was a problem hiding this comment.
@oliviertassinari @subjectix Since we merged #2562, maybe we could use that deprecatedProp utility function that was added. Thoughts?
There was a problem hiding this comment.
We have to do it for many other. And it's still not mature enough. I'd say wait a PR or two before we fully migrate to using that. I want to improve the doc generation in 2-3 ways. for functions, deprecated, and categorization.
There was a problem hiding this comment.
I want to improve the doc generation in 2-3 ways. for functions, deprecated, and categorization.
I agree, there is room for improvement. For instance by using the deprecatedPropType we could add more than just adding DEPRECATED to the doc description.
We could automatically add what to do instead (this is the warning that would be thrown if the property is used.
|
@subjectix Looking good! I made a couple of additional small comments that I'd be curious to get your feedback on. Otherwise I think it's good to go 👍 @subjectix @oliviertassinari Also, now that we've been naming things properly, I'm tempted to do another pull request naming |
|
@newoga Yes please do that 👍 👍 👍 Thanks 😁 |
|
what do you think @newoga ? 😁 |
|
|
||
| render() { | ||
|
|
||
| const items = longMenuItems.map((item, index) => ( |
There was a problem hiding this comment.
You can move this part outside the render method, no need for extra work.
There was a problem hiding this comment.
this isn't what makes it slow, it's the rendering. this extra work is pure js, and the iterations are only 100, this won't make much different, yet I think this is more expressive.
There was a problem hiding this comment.
Well, my point is, we can save 100 class memory allocation each time the value change.
But yes, it may be insignificant, I haven't any perf measurement to share 😁
There was a problem hiding this comment.
doesn't matter following your fix did make it look cooler, so I followed :D
|
@subjectix That's such an epic PR, great work 🎉. |
|
@subjectix Yup, I'm happy with this as well! 🎉 Good work! |
alitaheri
force-pushed
the
composable-drop-down-menu
branch
from
c70a32d
to
da4a685
Compare
|
@oliviertassinari @newoga Thanks for all your review and feedbacks 👍 👍 😄 |
[Docs] [DropDownMenu] [Composable] Migrate to new standard
@oliviertassinari @newoga @ajsharp @mbrookes
Closes #1487
Guys please take a look at this. I've deprecated, removed, and messed up the code.
@oliviertassinari I'll fix the prop description, tell me how it is so far.
@ajsharp I think this addressed your issue as well
Closes #2554.
I should change the entire docs, select field and a lot more. It's 4AM here, so tomorrow 😅 😴
P.S. @oliviertassinari Please go through the commits. I kept the history a bit clean to follow. I've removed some code that was in the way and had no effect what so ever! This PR will need a lot of discussion and a lot of review. Please take your time. this is a huge change, ( a lot more to come)