-
Notifications
You must be signed in to change notification settings - Fork 55
feat(menu): Replace shape prop with pills, pointing and underlined props #114
Conversation
Codecov Report
@@ Coverage Diff @@
## master #114 +/- ##
=======================================
Coverage 88.35% 88.35%
=======================================
Files 47 47
Lines 773 773
Branches 109 109
=======================================
Hits 683 683
Misses 87 87
Partials 3 3
Continue to review full report at Codecov.
|
have discussed with Miro. So, essentially, we have the following conditions that define the task:
Problem
Currently proposed solution
This leads us to the following things being possible <Menu pointing ... />
<Menu pointing='start' ... /> As a drawback, we have no restrictions over client to do the following things (with, generally speaking, undefined result): <Menu pointing underlined /> // mutually exclusive variants are mixed Alternative solution
With that the following way of declaring pointing start menu should be used: <Menu shape='pointing' pointingPosition='start' ... /> The main benefit of this approach is explicitness and consistency - there is still no way to mix exclusive variants of components, while complementary The drawback is mainly in amount of typing required - but change doesn't seem to be crucial. |
I tend to agree with Roman. Although it seems to go against the concept of mixed properties, I see the benefits of enum. |
A few thoughts on the Menu API:
|
We had a discussion about that today, with questions like what defines the border between component API (props) and a theme (variables), why underlined is a prop not a variable... When thinking about component API we must think out of box and consider not only different web themes but also completely different surfaces like native, even if we might never implement it, but just to find the correct abstraction. I completely agree on the And, personally, I have the same opinion on |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agreed to leave things as those are introduced (different props for each shape
) due to reasons expressed above - if necessary, this point will be rethought in future.
Menu
Breaking change: this replaces
shape='pills'|'pointing'|'underlined'
with three separate props:pills
,pointing
andunderlined
.The reason for that is that for vertical pointing menu, it must be possible to define pointer's direction (
start
vsend
) and the single shape enum is not enough.TODO
API Proposal