-
Notifications
You must be signed in to change notification settings - Fork 55
Conversation
Codecov Report
@@ Coverage Diff @@
## master #123 +/- ##
==========================================
- Coverage 69.17% 68.03% -1.15%
==========================================
Files 101 101
Lines 1340 1364 +24
Branches 259 272 +13
==========================================
+ Hits 927 928 +1
- Misses 411 434 +23
Partials 2 2
Continue to review full report at Codecov.
|
@@ -67,6 +96,16 @@ const menuItemStyles = { | |||
color: variables.defaultColor, | |||
}), | |||
...itemSeparator({ props, variables }), | |||
...(pointing && |
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.
the fact that string value for pointing
takes effect only for vertical menu seems to be a bit confusing. Maybe we should consider to introduce analog for horizontal one? At the same time, with that we, probably, will need to aknowledge that for horizontal one default value would be rather end
, as menu item's arrow is expected to point down by default:
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.
Currently we have no use for horizontal pointing
in Teams theme. Instead of adding it now, I would prefer adding it when there is a real requirement for that.
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.
And once we want to add support for top
and bottom
in pointing, I think it would make more sense to use start
and end
just for vertical and top
and bottom
for horizontal.
I understand you would like the properties to be universally valid across variants, but from user's POV it could be confusing. The whole library should consistently use top, end, bottom, start as direction.
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.
actually, cannot say that I completely agree with that. Let me, please, share my thoughts.
The whole library should consistently use top, end, bottom, start as direction.
While it could be a reasonable choice for some components, for other ones it seems to be not. For the sake of example, suppose that we would have a Layout
component similar to the one that currently exists, but with added capability to render elements vertically. In that case start
and end
, from user's POV, would make perfect sense to me - more than that, it would be much easier for me to change direction from horizontal
to vertical
,
// client pseudo-code
<Layout start={Foo} end={Bar} />
// changed to
<Layout start={Foo} end={Bar} vertical />
and with that be sure that all the inner composition of elements remains to be valid, i.e.
- the one that was rendered as
start
one is still the first one in Layout's column - the
end
one is rendered as last element in column
If it would be about having four properties from two different groups, it would be much more work for me as user, without any good reason (from user's perspective) for that.
Currently we have no use for horizontal pointing in Teams theme. Instead of adding it now, I would prefer adding it when there is a real requirement for that.
Agree that it is generally better to rely on requirements while deciding whether something should be introduced or not, but at the same time I think that when it is about breaking consistency from client's perspective (because, frankly, for me as a consumer it would be a point of sincere astonishment that start
and end
values of pointing
are applied only for vertical menu, while for horizontal one it is easily imaginable how those could be applied), and, at the same time, it is not hard to introduce fix - maybe it would be better to consider to introduce this change even if it is not mentioned in requirements.
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.
Ok, PR updated, horizontal pointing menu now honours start
/end
as well.
Menu vertical pointing
Vertical menu with items pointing either left or right.
stardust-ui/react-old#122
TODO
API Proposal
Vertical pointing
Vertical pointing menu points towards start:
It is possible for it to point towards end: