Conversation
varl
changed the title
varl
changed the title
| position: relative; | ||
| width: 100%; | ||
| margin: 0; | ||
| <div> |
There was a problem hiding this comment.
Should the className be added to the div or the MenuList? 🤔
|
I feel like atm we have one too many layer here, for example: I guess the reasoning can be summarised like this:
Personally I would prefer it if:
I think this many layers will just cause confusion. Also, in the current implementation what menu does is just render a Card with padding. So the component-name doesn't match what the component does. |
|
It's not a given that the DropMenu should include a Card though, and the design-system has a card-like wrapper around the Menu. |
It matches the name of the design system (https://github.com/dhis2/design-system/blob/master/atoms/menu.md) Which should probably answer all the other questions as well |
|
I'm still under the impression we are adding layers of fluff here, and that this implementation of
So according to the design system the Menu is the List |
|
Anyway, if you both feel strongly about going this, way: go for it. I will disagree and commit. |
The strongest feeling I have to this is backwards compatibility. I don't think changing the Other than that, what is driving this change is that the Any user that is using the This just allows us to use the It would be nice to avoid two extra |
|
Fair enough. OK. 👍 I would also like to see some prop validation here, so that the MenuList children should always be a single, or an array of MenuItem component instances. |
|
|
||
| MenuList.propTypes = { | ||
| className: propTypes.string, | ||
| children: propTypes.any.isRequired, |
There was a problem hiding this comment.
only accept instance(s) of MenuItem?
There was a problem hiding this comment.
That would be a breaking change.
# [3.4.0](dhis2/ui@v3.3.0...v3.4.0) (2019-06-21) ### Features * extract and expose MenuList from Menu component ([#297](dhis2/ui#297)) ([4d576af](dhis2/ui@4d576af))
|
🎉 This PR is included in version 3.4.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
4 participants
No description provided.