feat(header-navigation): add implementation by blackswanny · Pull Request #525 · uber/baseweb

blackswanny · 2018-11-20T00:56:13Z

add header navigation implementation

Header navigation is highly customizable component. All examples provide different ways customers can address their design and concerns. They mainly show flexibility of Base UI rather than component itself.

gergelyke · 2018-11-20T17:48:38Z

it seems like the hamburger icon is not in the middle

gergelyke

are you planning to add tests in a separate PR?

gergelyke · 2018-11-20T17:50:05Z

src/header-navigation/examples-list.js

+/* eslint-env node */
+/* eslint-disable flowtype/require-valid-file-annotation */
+module.exports = {
+  WITH_MENU_ELEMENT: 'With Dropdown Menu',


can you please add Header navigation (or something similar) to the names, so for the visual regression tests it is simpler to include in screener.config.js?

gergelyke · 2018-11-20T17:52:13Z

src/header-navigation/examples.js

+            })
+          }
+        >
+          <svg


can we move the svg to a file and import it, just to make this example more readable?

src/header-navigation/header-navigation.js

austingreendev · 2018-11-20T17:45:49Z

src/header-navigation/index.js

+// Styled elements
+export {
+  Root as StyledRoot,
+  NavigationItem,


Should we add Styled to the NavigationItem and NavigationList components for consistency?

As per @schnerd original proposal current names look more semantically right. I can rename it if needed

#514 (comment)

I agree with @Austin94 comment, we should be consistent. So far we export any styled component with Styled prefix. We should be consistent.

gergelyke · 2018-11-20T18:00:13Z

also, for the menu should we use point cursor instead of the text?

src/header-navigation/examples.js

blackswanny · 2018-11-20T21:47:21Z

it seems like the hamburger icon is not in the middle

@gergelyke
fixed

blackswanny · 2018-11-20T21:50:48Z

also, for the menu should we use point cursor instead of the text?

@gergelyke
Menu is used with it's own default styles, however I added cursor for better customization

blackswanny · 2018-11-20T21:51:54Z

are you planning to add tests in a separate PR?

yes, next commit

nadiia · 2018-11-20T22:48:57Z

Tab links in the example are not keyboard navigatable. Could you fix it please?

nadiia · 2018-11-20T22:52:52Z

src/header-navigation/examples.js

+
+import * as React from 'react';
+import {Button} from '../button';
+import {StatefulMenu as Menu, KEY_STRINGS} from '../menu';


We use menu component in the examples but the menu doesn't seem to work - never shows up in the current build.

nadiia · 2018-11-20T22:56:25Z

src/header-navigation/examples.js

+        <div
+          tabIndex="0"
+          aria-label="Navigation menu"
+          style={{lineHeight: 'initial'}}


It's only single style but may still not be a good example to show how we do styling since it created an inline styles. Let's use styled for adding styles.

nadiia · 2018-11-20T22:59:12Z

src/header-navigation/examples.js

+            </NavigationItem>
+            <NavigationItem>Uber</NavigationItem>
+          </NavigationList>
+          <NavigationList $align={ALIGN.center} />


What is this empty centered navigation list for?

So in my flexbox styles if all element are left or right docked flexbox still docks them together. I had to add this center to avoid it. I checked flexbox implementation and didn't find other solution. So there has to be at least one element, that is able to flexGrow.

nadiia · 2018-11-20T23:03:54Z

src/header-navigation/index.js

+export {
+  Root as StyledRoot,
+  NavigationItem,
+  NavigationList,


I wonder if we should create container components for both NavigationItem and NavigationList that will take overrides and render StyledNavigationItem and StyledNavigationList accordingly. Since this falls outside the pattern we used in other components so far. Same as we do for the HeaderNavigation.
@schnerd what do you think about it?

If we want to export it as a container we should add a container component to it

blackswanny · 2018-11-21T00:35:19Z

Tab links in the example are not keyboard navigatable. Could you fix it please?

@nadiia fixed

gergelyke · 2018-11-26T16:52:47Z

Can you update the cursor to use the pointer style instead of text?

gergelyke · 2018-11-28T00:10:37Z

also, can you please add the warning that says that this component is in a beta state, and may change without notice in the near future?

blackswanny · 2018-11-28T00:33:34Z

Can you update the cursor to use the pointer style instead of text?

updated for the whole header navigation

nadiia

Could you address the styled components vs container components comment please?

nadiia · 2018-11-28T19:39:22Z

src/header-navigation/index.js

+// Styled elements
+export {
+  Root as StyledRoot,
+  NavigationItem,


I agree with @Austin94 comment, we should be consistent. So far we export any styled component with Styled prefix. We should be consistent.

nadiia · 2018-11-28T19:39:54Z

src/header-navigation/index.js

+export {
+  Root as StyledRoot,
+  NavigationItem,
+  NavigationList,


If we want to export it as a container we should add a container component to it

blackswanny · 2018-11-28T21:25:21Z

Could you address the styled components vs container components comment please?

@nadiia renamed those to be styled components

nadiia · 2018-11-28T23:26:15Z

src/header-navigation/header-navigation.js

+  };
+  componentDidMount() {
+    // eslint-disable-next-line no-console
+    console.warn(


it should only warn in a DEV env

nadiia · 2018-11-28T23:28:37Z

src/header-navigation/examples.js

+      <React.Fragment>
+        <div style={{marginTop: '50px'}} />
+        <HeaderNavigation>
+          <NavigationList $align={ALIGN.left}>


Sorry, I must've missed this one on a previous look through. Why is this prop $ prefixed? It seems like there should be a container that would do the $ mapping

this is a styled component. If we add container, it would add complexity to maintain things.

I'm pretty torn on this. It could be confusing to users if some of the primary exports for a component require $ prefixes. On the other hand I agree creating wrappers every time we use this pattern could be annoying.

A couple ideas:

Write an HOC that wraps styled components and maps the $ props:

import {asPrimaryExport} from '../styled/as-primary-export-hoc'; export const StyledNavigationList = styled('div', {...}); export const NavigationList = asPrimaryExport(StyledNavigationList, ['align', 'prop2', 'prop3']);

This would be far less tedious than maintaining a wrapper component for every time we use this pattern.

Add logic to our styled implementation that supports similar prop mapping

export const StyledNavigationList = styled('div', {...}, { mapProps: ['align', 'prop2', 'prop3'] });

Internally I think this could use styletron's withWrapper? Still not 100% sure. Also adds more complexity to our styled implementation.

--

Obviously those are outside the scope of this specific PR, just trying to think of the best long term solution. Maybe short term we bite the bullet and build a container manually?

@schnerd Thanks for the proposals! I'd +1 the #1 HOC. Also should be easy to switch to this approach if meanwile we create containers manually.
@blackswanny what do you think?

apparently, my last comment wasn't submitted when I replied

I applied 1) approach. Currently need to fix the test using it

gergelyke · 2018-11-29T17:42:22Z

can you please use the theme variables for the hamburger icon too?

schnerd · 2018-11-20T01:34:38Z

src/header-navigation/header-navigation.js

+  render() {
+    const {overrides = {}} = this.props;
+    const [Root, rootProps] = getOverrides(overrides.Root, StyledRoot);
+    return <Root {...this.props} {...rootProps} />;


Should we use ...rest here so overrides isn't passed down to Root without a $ prefix?

I think, current code fixed it

schnerd · 2018-11-29T18:42:15Z

src/header-navigation/examples.js

+      <React.Fragment>
+        <div style={{marginTop: '50px'}} />
+        <HeaderNavigation>
+          <NavigationList $align={ALIGN.left}>


I'm pretty torn on this. It could be confusing to users if some of the primary exports for a component require $ prefixes. On the other hand I agree creating wrappers every time we use this pattern could be annoying.

A couple ideas:

Write an HOC that wraps styled components and maps the $ props:

import {asPrimaryExport} from '../styled/as-primary-export-hoc'; export const StyledNavigationList = styled('div', {...}); export const NavigationList = asPrimaryExport(StyledNavigationList, ['align', 'prop2', 'prop3']);

This would be far less tedious than maintaining a wrapper component for every time we use this pattern.

Add logic to our styled implementation that supports similar prop mapping

export const StyledNavigationList = styled('div', {...}, { mapProps: ['align', 'prop2', 'prop3'] });

Internally I think this could use styletron's withWrapper? Still not 100% sure. Also adds more complexity to our styled implementation.

--

Obviously those are outside the scope of this specific PR, just trying to think of the best long term solution. Maybe short term we bite the bullet and build a container manually?

blackswanny · 2018-11-29T19:54:33Z

can you please use the theme variables for the hamburger icon too?

fixed. @gergelyke

gergelyke

I am an A on this once David's feedback is resolved

chasestarr · 2018-11-29T23:12:20Z

Deploy preview for baseui ready!

Built with commit 178c457

https://deploy-preview-525--baseui.netlify.com

schnerd · 2018-11-29T23:31:04Z

src/styles/as-primary-export-hoc.js

+    }, {});
+    return <StyledComponent {...styledProps} />;
+  };
+}


This is awesome.

schnerd · 2018-11-29T23:35:25Z

src/header-navigation/styled-components.js

+    };
+  }),
+  ['align'],
+);


It might be nice to also flowtype this component since it's a primary export.

Flow, might complain that this is incompatible with return type of asPrimaryExport, but you could always cast through any to fix that.

export const NavigationList: React.ComponentType< {align: string, children: React.Node} > = (asPrimaryExport( ... ): any);

fixed. I wonder why flow didn't complain about it

nadiia

Before merging please make sure that the Styled component wrapped in HOC behaves as a styled component in cases of restyling

blackswanny · 2018-11-30T20:02:53Z

Before merging please make sure that the Styled component wrapped in HOC behaves as a styled component in cases of restyling

added example for that

blackswanny added the feature label Nov 20, 2018

blackswanny force-pushed the feature/header branch from 034767e to bbbc8e9 Compare November 20, 2018 00:57

gergelyke changed the title ~~add header navigation implementation~~ feat(header-navigation): add implementation Nov 20, 2018

gergelyke reviewed Nov 20, 2018

View reviewed changes

austingreendev reviewed Nov 20, 2018

View reviewed changes

blackswanny force-pushed the feature/header branch from bbbc8e9 to 8dc3fde Compare November 20, 2018 21:34

austingreendev reviewed Nov 20, 2018

View reviewed changes

src/header-navigation/examples.js Outdated Show resolved Hide resolved

blackswanny force-pushed the feature/header branch from 8dc3fde to a9740f5 Compare November 20, 2018 21:46

blackswanny force-pushed the feature/header branch 2 times, most recently from d2cffe0 to b365be5 Compare November 20, 2018 21:50

blackswanny force-pushed the feature/header branch from b365be5 to 0c50d27 Compare November 20, 2018 22:02

nadiia reviewed Nov 20, 2018

View reviewed changes

blackswanny force-pushed the feature/header branch 3 times, most recently from b6a4037 to 8690d87 Compare November 21, 2018 21:28

blackswanny force-pushed the feature/header branch 4 times, most recently from 2ed4ca7 to 010c33f Compare November 27, 2018 07:08

blackswanny force-pushed the feature/header branch from f3e9472 to 0b0126d Compare November 28, 2018 00:33

blackswanny force-pushed the feature/header branch 2 times, most recently from 0286047 to 96ade26 Compare November 28, 2018 19:30

nadiia suggested changes Nov 28, 2018

View reviewed changes

blackswanny force-pushed the feature/header branch 2 times, most recently from 90e1e54 to 98f4ff9 Compare November 28, 2018 21:24

nadiia reviewed Nov 28, 2018

View reviewed changes

blackswanny force-pushed the feature/header branch 2 times, most recently from af7260c to 459fc93 Compare November 29, 2018 17:41

schnerd reviewed Nov 29, 2018

View reviewed changes

blackswanny force-pushed the feature/header branch from 459fc93 to 98165d3 Compare November 29, 2018 19:54

gergelyke approved these changes Nov 29, 2018

View reviewed changes

blackswanny force-pushed the feature/header branch from 98165d3 to 79438de Compare November 29, 2018 23:10

blackswanny force-pushed the feature/header branch from 79438de to 178c457 Compare November 29, 2018 23:31

schnerd reviewed Nov 29, 2018

View reviewed changes

blackswanny force-pushed the feature/header branch 2 times, most recently from 6d7f501 to b7c4069 Compare November 30, 2018 01:05

nadiia approved these changes Nov 30, 2018

View reviewed changes

blackswanny force-pushed the feature/header branch from b7c4069 to ef44729 Compare November 30, 2018 20:02

Oleg Gomozov added 4 commits November 30, 2018 12:14

feat: add header navigation implementation

dd480ca

feat: add unit tests to header navigation

21cd5fe

feat: add HOC for props transformation to styled elements:

3544253

feat: add HOC for props transformation to styled elements:

3782e5d

blackswanny force-pushed the feature/header branch from ef44729 to 3782e5d Compare November 30, 2018 20:14

blackswanny merged commit d729075 into master Nov 30, 2018

baseui-probot-app-workflow bot deleted the feature/header branch November 30, 2018 21:21

Conversation

blackswanny commented Nov 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gergelyke commented Nov 20, 2018

Uh oh!

gergelyke left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gergelyke commented Nov 20, 2018

Uh oh!

Uh oh!

blackswanny commented Nov 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

blackswanny commented Nov 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

blackswanny commented Nov 20, 2018

Uh oh!

nadiia commented Nov 20, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

blackswanny commented Nov 21, 2018

Uh oh!

gergelyke commented Nov 26, 2018

Uh oh!

gergelyke commented Nov 28, 2018

Uh oh!

blackswanny commented Nov 28, 2018

Uh oh!

nadiia left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

blackswanny commented Nov 28, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

blackswanny commented Nov 20, 2018 •

edited

Loading

blackswanny commented Nov 20, 2018 •

edited

Loading

blackswanny commented Nov 20, 2018 •

edited

Loading

chasestarr commented Nov 29, 2018 •

edited

Loading