Dropdown mode for tabs

[amass01]

• tackles Tab alternative view for mobile screens #77

[amass01]

@fernandoabolafio
Have we created also an issue in politeiagui to reflect the changes from this PR there ?

[fernandoabolafio]

@amassarwi there is no issue for that in politeiagui yet. We can add one once this is done.
As I mentioned in this issue what I expected is that by simply adding a prop "dropdownMode" to Tabs it would transform itself to become a vertical selector. It's very confusing to me that to achieve this I have to first place the Tabs inside a Dropdown and just then set the "dropdownMode" as true.
So if I set "dropdownMode" for a Tabs out of a dropdown, nothing happens?

Let's clarify some things here to get this fixed:

• These Tabs feature has nothing to do with the dropdown component, so the usage of the component needs to move into the Tabs docs and out of Dropdown docs.
• It's ok to import the Dropdown component in the Tabs component if you need it to achieve the desired output. However, don't expect the other devs to figure that they need to place a Tabs inside a dropdown for it to work. I suggest we do the following approach for the Tabs component:

<Tabs mode={"horizontal" (default) | "vertical" | "dropdown" } />

Just by changing the mode prop the Tabs view needs to react accordingly.

[amass01]

@fernandoabolafio Thanks for your feedback, I'll change my implementation as you described, it makes sense.

[amass01]

@fernandoabolafio - I went in the direction you described, the implementation is not 100% ready, but I would like to get your feedback - if that's what you imagined before finalizing the task.

There is a small visual jump on tab switch which I would like to fix, and the dropdown anchor is not part of the custom trigger and will need to add it (shown in design spec)

[fernandoabolafio]

@amassarwi it looks way better now! Thanks for updating it. That's indeed how I imagined before and the usability of the component looks great now.
I noticed the jump, go ahead with your final fixes. I have two suggestions/heads up for you:

• In the specs, the tab list items are aligned left.
• For the dropdown arrow, you may want to modify the Dropdown component, so the arrow is kept even when you pass a custom trigger. This would make your life b/c you don't need to reimplement the arrow. If we need, in the future, we can add a prop to show/hide the arrow.

[fernandoabolafio]

@amassarwi looking good. Besides the 'closeOnClick' you are aware of I found one code improvement to be made. After those being addressed, this is good to go.

[fernandoabolafio]

You can avoid some duplicating the <ul>...</ul> code here by defining the list before the return statement. Something like this:

cons Tabs = ({...}) => {
  ...
  const tabs = (
          <ul
            className={classNames(
              vertical ? styles.tabsNavVertical : styles.tabsNav,
              wrap && styles.wrap,
              className
            )}
            style={style}
            {...props}>
            {renderChildrenTabs()}
          </ul>)
  ...
  return (
   <>
    {dropdownMode ? <Dropdown ... />{tabs}</Dropdown> : tabs }
     ...
   </>);
}

[fernandoabolafio]

Just to give you some tips on react performance. In the chunk of code I placed above you could wrap it with the useMemo hook from react. This would memoize the value of tabs and only reevaluate the statement if one of its arguments has changed. Something like:

const Tabs = ({...}) => {
...
  const tabs = useMemo(() => (
          <ul
            className={classNames(
              vertical ? styles.tabsNavVertical : styles.tabsNav,
              wrap && styles.wrap,
              className
            )}
            style={style}
            {...props}>
            {renderChildrenTabs()}
          </ul>), 
[vertical, wrap, className, props, renderChildrenTabs]);
...
return (...)
}

This will cause the tabs value to be computed only if one of the arguments passed to the array (vertical, wrap, className, props, renderChildrenTabs) changes.
As you may notice the renderChildrenTabs will always have a different value since this function is being redefined every time the component is rendered. To avoid that you could use the useCallback:

const Tabs = ({...}) => {
  const renderChildrenTabs = useCallback(() => 
     React.Children.map(children, (child, index) => {
      return React.cloneElement(child, {
        onSelect: onSelectTab,
        tabIndex: index,
        isActive: index === activeTabIndex,
        mode
      });
   ), [children, activeTabIndex, mode]);
  ...
  const tabs = useMemo(() => (
          <ul
            className={classNames(
              vertical ? styles.tabsNavVertical : styles.tabsNav,
              wrap && styles.wrap,
              className
            )}
            style={style}
            {...props}>
            {renderChildrenTabs()}
          </ul>), 
[vertical, wrap, className, props, renderChildrenTabs]);
...
return (...)
}

useCallback will return a memoized version of the callback that only changes if one of the dependencies has changed. This is useful when passing callbacks to optimized child components that rely on reference equality to prevent unnecessary renders

[amass01]

@fernandoabolafio both comments were addressed

[fernandoabolafio]

tACK