PageHeader component draft #60

davidtheclark · 2018-11-30T19:01:40Z

Draft component for a shareable page header. I want to confirm the appearance with anyone interested before finishing it off with tests and documentation. I'm imagining this will be used in apps owned by @mapbox/studio and @mapbox/accounts. Please share your ideas about how this looks and works, here or in chat!

Here's what you get:

Non-mobile
- Mapbox logo on the left linking to the homepage.
- Just to the right of that, the name of the app, linking to the app subdomain home (href="/").
- Single-level navigation (i.e. no dropdowns).
- Space on the far right for us to insert the user menu trigger.
Mobile
- App nav trigger on the left, Mapbox mark and app name between, user menu trigger on the right.
- App nav opens in a panel on the left. Has the Mapbox mark and app name at the top, links beneath, close button on the right.
- When app nav is open, there's a dark background over the page, and when you touch it the nav closes.
Mobile header shows when you're under the -mm breakpoint, non-mobile header shows above that.

Pictures:

cc @mapbox/frontend @mapbox/frontend-platform

davidtheclark · 2018-11-30T19:11:13Z

cc @mapbox/docs to take a look, for ideas, etc., though you'll need a different implementation.

elifitch · 2018-12-03T15:45:48Z

More of a design concern, but I'd love it if the hamburger menu button and the close menu button were in the same spot. Makes it a little more convenient to use.

davidtheclark · 2018-12-03T19:23:33Z

@elifitch Thanks for chiming in. We can do that. My concern is that it then puts the logo + sitename in a weird place on the right. Suggestion: if we want the close button on the left, we make the menu open up to the full width of the viewport, extending down only as far as it needs to go. What do you think of that?

OR we have the slider slide out beneath the header, instead of on top of it.

elifitch · 2018-12-03T20:21:48Z

OR we have the slider slide out beneath the header, instead of on top of it.

This seems like a nice compromise :)

lshig · 2018-12-03T22:46:46Z

OR we have the slider slide out beneath the header, instead of on top of it.

In doing this, do you imagine that the hamburger menu icon will animate or fade into the x icon? I like how our current mobile menu keeps the Mapbox logo and the icon in the same place.

cc: @angel

lshig · 2018-12-03T23:07:20Z

Also, question related to the User placeholder, will the user menu also have a mobile version slider or will it remain a dropdown type menu in desktop and mobile screens? If we do the slider as mentioned previously, I'm having a hard time imagining how those 2 menus will compete with each other for space. Will they overlap? Will one animate in and the other animate out of view?

ahelkit · 2018-12-03T23:10:13Z

Could we have the user be a part of the menu then the menu can be back on the right hand side and the logo can stay on the left, and nothing has to move?

davidtheclark · 2018-12-03T23:18:58Z

Initial version will have no animation, unless @mapbox/accounts or @mapbox/studio has the opportunity to contribute that feature. Animation can always be added later.

User menu will be a dropdown, as it is now, rather than a (non-sliding) slider. This way it can work essentially the same on desktop and mobile.

Idea: the app menu could also be a dropdown. That could provide some nice symmetry between the two menus.

If you click outside one of the menus, it should close. So if you have the user menu open and you click the app menu trigger, the user menu should close.

Could we have the user be a part of the menu then the menu can be back on the right hand side and the logo can stay on the left, and nothing has to move?

I don't think so, unless we were to scratch the user menu idea. The user menu is a standalone piece that @mapbox/frontend-platform will maintain. I don't know of a technically reasonable way for us to blend that into the subdomain-specific menus for apps and docs.

danswick · 2018-12-11T23:24:42Z

I made some tweaks based on the discussion in this ticket and would love feedback from folks. The biggest change was moving to a simple dropdown from a non-sliding slider per #60 (comment). I tried to make it look as much like the user menu as possible without going full pointy-popover and I'm not sure if it works well or if it's weird.

davidtheclark · 2018-12-12T18:21:23Z

@danswick Positioning looks good. I think it needs more work to match the existing dropdowns. I can tell it lacks the triangle and the padding of the existing dropdowns. Maybe that's it?

danswick · 2018-12-12T23:38:41Z

Thanks for the feedback, @davidtheclark. The latest commit adds a dynamically-positioned pointer triangle, updates padding to match that of the other dropdowns in the existing page shell, and cleans up some CSS nonsense from my previous commit.

davidtheclark · 2018-12-13T14:30:00Z

@danswick looks like good progress. A couple of more visual points before we look at the code:

The triangle is the same width and height as the user menu's. Right now it's wider and taller.
The offset from the bottom of the trigger is the same as the user's menu. Right now it's higher, I think.

danswick · 2018-12-13T22:33:47Z

Pointer and spacing should be pretty close now, @davidtheclark. For the vertical position, I made a temporary change to initialize the user menu and compared the two.

davidtheclark

Good progress here. I looked at the code and the demo. The demo looks good and works, so we're on the right track.

davidtheclark · 2018-12-13T22:36:16Z

src/components/page-header/page-header.js

+    });
+
+    return (
+      <div className="limiter flex-parent-mm flex-parent--center-cross">


flex-parent-mm could just flex-parent, I think.

davidtheclark · 2018-12-13T22:37:21Z

src/components/page-header/page-header.js

+          style={{ width: 66 }}
+          className="flex-child py6 round-full border color-blue txt-bold align-center txt-s"
+        >
+          User


I don't think we want this placeholder.

davidtheclark · 2018-12-13T22:37:30Z

src/components/page-header/page-header.js

+        <div
+          id="mbx-user-menu"
+          style={{ width: 66 }}
+          className="flex-child py6 round-full border color-blue txt-bold align-center txt-s"


None of these are necessar except flex-chidl, right?

davidtheclark · 2018-12-13T22:38:05Z

src/components/page-header/page-header.js

+  }
+
+  closeOnDocumentClick = event => {
+    // Close the popup if the target is me


davidtheclark · 2018-12-13T22:38:27Z

src/components/page-header/page-header.js

+    // Close the popup if the target is me
+    if (
+      this.state.open &&
+      event.target instanceof Node &&


Can we remove this line? I think @jseppi only included it in user menu code to please Flow.

davidtheclark · 2018-12-13T22:42:39Z

src/components/page-header/page-header.js

+  render() {
+    return (
+      <div className="relative">
+        <div className="limiter flex-parent flex-parent--center-cross flex-parent--space-between-main">


Re flex-parent--space-between-main: We'll want to change the positioning so the logo & name is left-aligned, matching the recent move in the docs-page-shell.

davidtheclark · 2018-12-13T22:42:52Z

src/components/page-header/page-header.js

+            className="flex-child py6 round-full border color-blue txt-bold align-center txt-s"
+          >
+            User
+          </div>


Some comments on this element as above.

davidtheclark · 2018-12-13T22:43:59Z

src/components/page-header/page-header.js

+    if (
+      this.state.open &&
+      event.target instanceof Node &&
+      !this.node.contains(event.target)


If this.node is the <button> node, doesn't that mean the menu will close when you click inside it (but maybe not on a link)?

I think we have the following requirements when the menu is open:

If you click on a link in the menu, it closes.

If you click the trigger, it closes.

If you click outside the menu, it closes.

If you click inside the menu but not on a link (e.g. you miss and hit some anodyne whitespace), it does not close.

davidtheclark · 2018-12-13T22:47:01Z

src/components/page-header/__tests__/page-header-test-cases.js

+      },
+      {
+        href: '/foo',
+        text: 'Another item with a long name'


Let's cut this because it doesn't match a real use case. These names kind of need to be shortish. Instead, it would be good to test the width limits of this by trying the longer menu from Account Dashboard.

davidtheclark · 2018-12-13T22:50:24Z

src/components/page-header/page-header.js

+    return (
+      <div
+        id="mobile-menu-container"
+        className="absolute left shadow-darken10-bold bg-white round"


Let's offset this from the left side just a little bit — 10 pixels, like the user menu body.

danswick · 2018-12-14T23:42:30Z

After adding a new test case for Accounts, it looks like we may need to consider some design options in order to make things fit.

The following screenshots are taking with page width set to 640 pixels, the cutoff for our mobile breakpoint (note that the plain component doesn't actually have a visual user menu element, so I added a silly blue box to help you visualize the spacing):

Here it is with nothing changed 😭

Here it is with some item name tweaks.

Removed "Dashboard." With the new account subdomain, you can just click on the big "Account" page name to go back to the dashboard.
Changed "Access tokens" to "Tokens."
Changed "Statistics" to "Stats."

For reference, the full complement of Nav items starts to look normal at around 880 pixels wide:

Some ideas

Here are some things we could try to make more room:

Set a higher mobile breakpoint so that the hamburger menu shows up on larger screens.
Have the mapbox part of the wordmark disappear at some point to make room for more menu items.
Expand the nav to a second row at some width.
Dynamically add items to a "more" dropdown like this Microsoft documentation site. This will be more difficult to implement and I'm not sure it's a great idea to hide any of the Nav items for Account.

@mapbox/accounts @angel - would you mind chiming in with your reactions, feedback, and preferences for how we might better fit these items into the Account nav?

ahelkit · 2018-12-14T23:48:06Z

I vote for 2. Since we cut that off on mobile anyways, I think it would be ok to cut if off a little earlier, but I like have the links still available at the larger screen size to tap easily.

davidtheclark · 2018-12-14T23:59:22Z

src/components/page-header/page-header.js

+        <nav className="flex-child flex-child--grow flex-parent flex-parent--center-cross flex-parent--end-main txt-bold txt-s mt6">
+          {itemEls}
+        </nav>
+        <div id="mbx-user-menu" style={{ width: 66 }} className="flex-child" />


This needs flex-child--no-shrink to ensure it's never narrower than 66px.

I noticed it got shrunk on one of the screenshots you posted.

danswick · 2018-12-15T00:12:48Z

Thanks, @angel! How do you feel about dropping "Dashboard" and relying on the blue "Account" link to get users back to the main account page? If we dynamically remove "mapbox," this is as cramped as it gets:

followed by:

and then the most cramped before going mobile hamburger:

edit: also wanted to note that users will have another link back to their main account page from the user menu!

edit2: after some rigorous IRL breakpoint testing with @angel, we came up with the following mix of:

Nixing the "mapbox" part of the word mark, along with the blue pipe, disappear below a certain size.
Trimming the "Access" off of "Access Tokens."

The gif is a bit jumpy, but "Dashboards" never gets closer than about 15 pixels to "Account."

davidtheclark

Looks like there are some z-index problems:

Also: I can't seem to open the mobile nav on the second example (Account).

davidtheclark · 2018-12-17T21:39:05Z

src/components/page-header/__tests__/page-header.test.js

+  test('renders as expected', () => {
+    const wrapper = shallow(
+      React.createElement(testCase.component, testCase.props)
+    );


Let's be as consistent as possible within a file about whether we create the wrapper in the beforeEach hook or in the test block.

davidtheclark · 2018-12-17T21:40:19Z

src/components/page-header/page-header.js

+      <div
+        id="mobile-menu-container"
+        className="absolute left shadow-darken10-bold bg-white round"
+        style={{ top: '56px', marginLeft: MOBILE_NAV_MENU_OFFSET_LEFT }}


Where does the magic number 56 come from? Can we make it a named constant?

davidtheclark · 2018-12-17T21:41:30Z

src/components/page-header/page-header.js

+          </div>
+          <div
+            id="mbx-user-menu-mobile"
+            style={{ width: 66 }}


Let's make this magic 66 a named constant, too.

davidtheclark · 2018-12-17T21:43:01Z

src/components/page-header/page-header.js

+      <div className="limiter flex-parent flex-parent--center-cross">
+        <div className="flex-child flex-child--no-shrink flex-parent flex-parent--center-cross">
+          <a
+            className="flex-child mb-logo wmax180-mxl wmax30-mm"


Can wmax30-mm be wmax30, to simplify things?

davidtheclark · 2018-12-17T21:43:44Z

src/components/page-header/page-header.js

+          </div>
+        </div>
+
+        <nav className="flex-child flex-child--grow flex-parent flex-parent--center-cross flex-parent--end-main txt-bold txt-s mt6">


What is mt6 for? It looks to me like it throws off the vertical centering.

I think it looks kind of weird to have different sizes of text vertically centered, so I aligned their baselines instead (tho I'm sure there's a better way to do it than with hardcoded margins). Though I guess everything else in the page shell is centered vertically and my stance on the issue isn't super strong, so I can change it back.

I'm fine with whatever @angel and @mapbox/studio want. And if there's solid consensus we should implement the same alignment in docs-page-shell.

I'm a fan of center alignment cause it would also be centered to the logo, button, and avatar, but can do this way if Studio feels more strongly about it.

sounds good. switching back to centered text 👍

danswick · 2018-12-17T22:29:14Z

@davidtheclark I don't think we can have two functional test cases at the same time without reworking some aspects of the component. Because there are multiple components rendered on the same page, there are two popup triggers with the same ID, so closeOnDocumentClick ignores clicks on the second menu trigger. I'm not sure if this is something we'd want to address because there should never be more than one of these components on a single page.

davidtheclark · 2018-12-17T22:43:45Z

@danswick The document-wide click event listener that causes the closing is currently added when the whole header mounts, removed when it unmounts. What if, instead, it was added only after the menu opened, then removed after the menu closed? This would also prevent there from being an unnecessary click handler attached to the document at all times.

davidtheclark · 2018-12-17T22:47:19Z

Another point to address would be to use a different element attribute instead of ID.

It's true that this point wouldn't be worth spending too much time on because it isn't realistic. We could have just one test cases that pushes the limits.

danswick · 2018-12-18T01:20:35Z

Ok! I think I got everything in here:

Text is re-centered vertically.
The mobile menu trigger is using refs instead of IDs now.
Document click listers are added when the menu is created and rendered when the menu is closed.
CSS is cleaned up.
Tests are consistified.
Magic numbers are const'd.

Going to merge, though I'm a bit disappointed we didn't hit 50 comments.

PageHeader component draft

6ec79c0

danswick added 2 commits December 5, 2018 17:03

mobile menu to dropdown

42b0dcd

update page header styles

36fc46e

pythagorean update

af495df

danswick added 3 commits December 13, 2018 14:06

add styles

762d994

add mbx-user-menu ID to the User placeholder oval

d778e0b

tweak vertical position

7734323

davidtheclark commented Dec 13, 2018

View reviewed changes

danswick and others added 4 commits December 14, 2018 12:56

review updates

86c5f7e

Simpler pointer positioning

2144199

update mobile nav pointer width

86d0546

add page header and footer legal strip to changelog

5574511

add snapshots and test

ab9d688

davidtheclark commented Dec 15, 2018

View reviewed changes

danswick added 2 commits December 14, 2018 16:53

update styles to accommodate more nav items

47019f3

update snapshots

d1584e5

davidtheclark commented Dec 17, 2018

View reviewed changes

address a few more comments and update assembly version

e4389a8

danswick merged commit da41170 into master Dec 18, 2018

danswick deleted the page-header branch December 18, 2018 01:27

davidtheclark mentioned this pull request Dec 18, 2018

Vertically center navigation text in PageHeader #64

Closed

danswick mentioned this pull request Dec 18, 2018

In PageHeader, doc click event listener should be attached and removed in lifecycle hooks, not render methods #65

Closed

PageHeader component draft #60

PageHeader component draft #60

Uh oh!

Conversation

davidtheclark commented Nov 30, 2018

Uh oh!

davidtheclark commented Nov 30, 2018

Uh oh!

elifitch commented Dec 3, 2018

Uh oh!

davidtheclark commented Dec 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elifitch commented Dec 3, 2018

Uh oh!

lshig commented Dec 3, 2018

Uh oh!

lshig commented Dec 3, 2018

Uh oh!

ahelkit commented Dec 3, 2018

Uh oh!

davidtheclark commented Dec 3, 2018

Uh oh!

danswick commented Dec 11, 2018

Uh oh!

davidtheclark commented Dec 12, 2018

Uh oh!

danswick commented Dec 12, 2018

Uh oh!

davidtheclark commented Dec 13, 2018

Uh oh!

danswick commented Dec 13, 2018

Uh oh!

davidtheclark 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!

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!

danswick commented Dec 14, 2018

Some ideas

Uh oh!

ahelkit commented Dec 14, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danswick commented Dec 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

davidtheclark 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

davidtheclark commented Dec 3, 2018 •

edited

Loading

danswick commented Dec 15, 2018 •

edited

Loading