feat(MenuItem|AccordionTitle): add ability for removing and customizing the (active|submenu) indicator #721

mnajdova · 2019-01-14T17:45:08Z

This PR aims to fix #687 (allowing the user to remove the submenu indicator), as well is is adding props for customizing the indicator, by providing an icon shorthand. I have few proposal for the API, and would like to hear your opinion on them.

Add explicit props for all things

hideSubmenuIndicator: boolean - indicartes whether the submenu indicators should be shown
submenuIndicatorVertical: ShorthandValue - shorthand for icon for the submenu indicators in the vertical menus
submenuIndicatorHorizontal: ShorthandValue - shorthand for icon for the submenu indicators in the horizontal menus

Pros: this API provides you to define all indicators in the root menu, and all these will be propagated to all submenus (vertical and horizontal)
Cons: too many properties added for just one slot

Add single property

submenuIndicator: boolean | ShorthandValue - this is a single prop that defined whether the submenu indicator shuold be shown (true or false), but also allows the user to specify shorthand for the icon in it.

Props: we are adding just one prop for the submenu indicator, so the user doesn't have to think which one he should apply.
Cons: with this approach, we cannot specify on the root menu what should the submenu indicators in all menus be, as the vertical and horizontal menus are most likely to be different icons. This means, every menu shuold define the submenu indicators in it's first children MenuItems.

3. Add single property and transform the icon - this is currently implemented

submenuIndicator: boolean | ShorthandValue - this is a single prop that defined whether the submenu indicator shuold be shown (true or false), but also allows the user to specify shorthand for the icon in it. For the vertical menus, we can transform this icon 90 degrees.

Props: we are adding just one prop for the submenu indicator, so the user doesn't have to think which one he should apply. Allows the user to specify the submenu indicator on the root menu
Cons: not sure whether this would be expected behavior for the client.

TODO:

update changelog
add Indicator examples

… for the vertical and horizontal menus -TODO rethink the names used

src/components/Menu/MenuItem.tsx

-supporting rtl for the icons -added submenu and active indicators in the Menu and AccordionTitle components

sophieH29 · 2019-01-15T13:46:14Z

Thought that came into my mind, users might want to have different icons for open/close state. Do we consider to support it too?

Anyway, I would vote for option 2

mnajdova · 2019-01-16T11:04:07Z

@sophieH29 thanks for your thoughts. This is currently possible, by adding styles for the MenuItem, not sure if we want to blow up the API, by adding additional things at this moment. If this comes as a requirement in the future, sure we can extend the API. However at this moment, I would restrain from adding more props. Do you have strong opinion that we want to support this at this moment?

mnajdova · 2019-01-16T11:38:47Z

src/components/Menu/MenuItem.tsx

+      vertical,
+      submenuIndicator,
+    } = this.props
+    const submenuIndicatorWithDefault = submenuIndicator === undefined ? true : submenuIndicator


We cannot specify the default value of the submenuIndicator in the defaultProp, as it is itemShorthand and conflicts with the children. If anybody has other idea, please let me know.

src/components/Accordion/AccordionTitle.tsx

sophieH29 · 2019-01-16T16:47:24Z

@sophieH29 thanks for your thoughts. This is currently possible, by adding styles for the MenuItem, not sure if we want to blow up the API, by adding additional things at this moment. If this comes as a requirement in the future, sure we can extend the API. However at this moment, I would restrain from adding more props. Do you have strong opinion that we want to support this at this moment?

Don't have a strong opinion that we need to support it right now, rather out of curiosity and something that we can keep in mind.

src/themes/teams/components/Icon/iconStyles.ts

codepretty · 2019-01-16T19:18:45Z

My vote is for the 3rd option, it seems the most minimal to get the job done.

src/themes/base/components/Indicator/indicatorStyles.ts

src/components/Indicator/Indicator.tsx

sophieH29 · 2019-01-17T10:22:27Z

src/components/Indicator/Indicator.tsx

+
+export interface IndicatorProps extends UIComponentProps, ColorComponentProps {
+  /** The indicator can be direction in different directions. */
+  direction?: 'forward' | 'back' | 'top' | 'bottom'


For me, it seems a bit confusable, what is 'bottom' and what is 'back'. I guess it's left but seems not very intuitive.
Maybe just 'right', 'left', 'top', 'bottom' - simple and straightforward

right and left are not RTL safe.

Would like to avoid the right and left, as in rtl it won't make sense to use them.. I am open to other suggestion. This one were gathered from the browser arrows for navigation (back and forward). Maybe start and end? What do you think?

Well, we use 'right' and 'left' for the Popup too.
I don't think start end is better as it more describes a position in regards to something. If no other ideas then let's leave 'back' and 'forward'. If treat it as video player buttons it makes more sense 😄

I vote for start and end as these are the direction names we use.

@miroslavstastny agree, let's strive for consistency, although it seems really strange for me to have the '<' indicator a start, and '>' as end.. We can change it later anyway...

src/components/Indicator/Indicator.tsx

-refactored icon styles to be more readable

-added examples

alinais · 2019-01-17T11:46:51Z

@sophieH29, @mnajdova I don't think it is in the scope of this fix to allow other icon variants for the submenu. Let's create a separate feature and address it separately.

mnajdova · 2019-01-17T14:25:22Z

@sophieH29, @mnajdova I don't think it is in the scope of this fix to allow other icon variants for the submenu. Let's create a separate feature and address it separately.

@alinais I agree is not the scope of this fix, but I would rather create another issue and address it as part of this PR, as otherwise we will add just one prop for removing the indicator and then we will introduce breaking changes for refactoring it to this implementation..

-moved the rotate icon prop from default to override props in the Indicator component

# Conflicts: # src/components/Menu/MenuItem.tsx # src/themes/teams/components/Menu/menuItemStyles.ts

-remove helper methods for getting the mapping arrow in rtl

docs/src/examples/components/Indicator/Types/IndicatorExampleDirection.shorthand.tsx

docs/src/examples/components/Indicator/index.tsx

layershifter · 2019-01-21T11:15:32Z

src/components/Icon/Icon.tsx

@@ -37,6 +37,9 @@ export interface IconProps extends UIComponentProps, ColorComponentProps {
  /** Name of the icon. */
  name?: string

+  /** An icon can be rotated by the degree specified as number. */
+  rotate?: number


String looks like a valid value, too:

<Indicator rotate='90' />

The rotate property of the icon can be used for calculation as well, take a look in the Indicator component (the overrideProps function for the Icon). In that case we will need to cast this to number if it was provided as string. Do we really want this?

src/components/Indicator/Indicator.tsx

# Conflicts: # CHANGELOG.md

manajdov added 2 commits January 14, 2019 18:05

-added option for hiding the submenu indicators as well as shorthands…

fe22c57

… for the vertical and horizontal menus -TODO rethink the names used

-refactored menu item styles

6b3a2e1

mnajdova requested review from dvdzkwsk, kuzhelov, layershifter, levithomason and miroslavstastny as code owners January 14, 2019 17:45

mnajdova added the 🚧 WIP label Jan 14, 2019

mnajdova changed the title ~~feat(Menu): added prop for hiding the submenu indicator, as well as shorthands for it (vertical and horizontal)~~ feat(Menu): added hideSubmenuIndicator prop, submenuIndicator[Vertical|Horizontal] shorthand for icon Jan 14, 2019

layershifter reviewed Jan 14, 2019

View reviewed changes

src/components/Menu/MenuItem.tsx Outdated Show resolved Hide resolved

mnajdova changed the title ~~feat(Menu): added hideSubmenuIndicator prop, submenuIndicator[Vertical|Horizontal] shorthand for icon~~ feat(Menu): add ability for removing and customizing the submenu indicator Jan 15, 2019

manajdov added 2 commits January 15, 2019 12:37

-added rotate prop to the Icon

740e44f

-supporting rtl for the icons -added submenu and active indicators in the Menu and AccordionTitle components

-removed boolean from the types of the indicators

0d1f9fb

manajdov added 2 commits January 15, 2019 16:19

-added UnicodeCharacter component

8e7c437

-replaced style unicode characters with UnicodeCharacter usages

48eca8b

-fixed tests

00738b4

mnajdova changed the title ~~feat(Menu): add ability for removing and customizing the submenu indicator~~ feat(MenuItem|AccordionTitle): add ability for removing and customizing the (active|submenu) indicator Jan 16, 2019

mnajdova commented Jan 16, 2019

View reviewed changes

layershifter reviewed Jan 16, 2019

View reviewed changes

src/components/Accordion/AccordionTitle.tsx Outdated Show resolved Hide resolved

-replaced UnicodeCharacter with Indicator

55ec1bd

codepretty reviewed Jan 16, 2019

View reviewed changes

src/themes/teams/components/Icon/iconStyles.ts Outdated Show resolved Hide resolved

codepretty reviewed Jan 16, 2019

View reviewed changes

src/themes/base/components/Indicator/indicatorStyles.ts Outdated Show resolved Hide resolved

codepretty reviewed Jan 16, 2019

View reviewed changes

src/components/Indicator/Indicator.tsx Outdated Show resolved Hide resolved

sophieH29 reviewed Jan 17, 2019

View reviewed changes

src/components/Indicator/Indicator.tsx Outdated Show resolved Hide resolved

-renamed indicator* props to indicator

a20f12b

-refactored icon styles to be more readable

-removed color prop in the indicator

670ee76

-added examples

manajdov added 5 commits January 17, 2019 15:38

-renamed forward and back direction to start and end

4c7cbd9

-moved the rotate icon prop from default to override props in the Indicator component

Merge branch 'master' into feat/menu-item-hide-submenu-indicator

a4a58ac

# Conflicts: # src/components/Menu/MenuItem.tsx # src/themes/teams/components/Menu/menuItemStyles.ts

-updated changelog

1073c5e

-remove default props in AccordionTitle

3b481f7

-removed arrow unicode chars from the teams theme

ad4040a

-remove helper methods for getting the mapping arrow in rtl

mnajdova added 🚀 ready for review and removed 🚧 WIP labels Jan 18, 2019

layershifter reviewed Jan 21, 2019

View reviewed changes

docs/src/examples/components/Indicator/Types/IndicatorExampleDirection.shorthand.tsx Outdated Show resolved Hide resolved

layershifter reviewed Jan 21, 2019

View reviewed changes

docs/src/examples/components/Indicator/index.tsx Outdated Show resolved Hide resolved

layershifter reviewed Jan 21, 2019

View reviewed changes

src/components/Indicator/Indicator.tsx Outdated Show resolved Hide resolved

manajdov added 3 commits January 21, 2019 15:54

Merge branch 'master' into feat/menu-item-hide-submenu-indicator

7c445c2

# Conflicts: # CHANGELOG.md

-addressed comments on the PR

048bc08

-replaced div with fragment

2426e20

layershifter approved these changes Jan 21, 2019

View reviewed changes

Merge branch 'master' into feat/menu-item-hide-submenu-indicator

4c2fd8b

mnajdova merged commit 519fda8 into master Jan 21, 2019

alinais deleted the feat/menu-item-hide-submenu-indicator branch January 21, 2019 15:47

layershifter mentioned this pull request Feb 13, 2019

Icon|Dropdown: components use an icon as indicator #896

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(MenuItem|AccordionTitle): add ability for removing and customizing the (active|submenu) indicator #721

feat(MenuItem|AccordionTitle): add ability for removing and customizing the (active|submenu) indicator #721

mnajdova commented Jan 14, 2019 •

edited

Loading

sophieH29 commented Jan 15, 2019 •

edited

Loading

mnajdova commented Jan 16, 2019

mnajdova Jan 16, 2019

sophieH29 commented Jan 16, 2019

codepretty commented Jan 16, 2019

sophieH29 Jan 17, 2019

layershifter Jan 17, 2019

mnajdova Jan 17, 2019

sophieH29 Jan 17, 2019

miroslavstastny Jan 17, 2019

mnajdova Jan 17, 2019

alinais commented Jan 17, 2019

mnajdova commented Jan 17, 2019

layershifter Jan 21, 2019

mnajdova Jan 21, 2019

feat(MenuItem|AccordionTitle): add ability for removing and customizing the (active|submenu) indicator #721

feat(MenuItem|AccordionTitle): add ability for removing and customizing the (active|submenu) indicator #721

Conversation

mnajdova commented Jan 14, 2019 • edited Loading

sophieH29 commented Jan 15, 2019 • edited Loading

mnajdova commented Jan 16, 2019

Choose a reason for hiding this comment

sophieH29 commented Jan 16, 2019

codepretty commented Jan 16, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alinais commented Jan 17, 2019

mnajdova commented Jan 17, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mnajdova commented Jan 14, 2019 •

edited

Loading

sophieH29 commented Jan 15, 2019 •

edited

Loading