Masterbar: clean up, component refactor, and move to gridicons #631

mtias · 2015-11-24T14:19:49Z

This issue is a deep refactoring of the Masterbar component.

Summary:

Modular, normalized the components to just masterbar-item* and derivates.
Restructured the DOM tree from 7-levels to 2-levels.
Rewritten the SCSS almost entirely, using flex magic.
Switched to ES6 syntax.
Ready for logged-out / SSR masterbar work.
Switches to Gridicons.

To test:

Do a fresh load (cache off / cleared)
Navigate to the top five sections (My Sites, Reader, New, Me, Notifications)
Pay attention at the loader dot the first time each section loads, and see if there are glitches
Check that notifications work (orange bubble appearing and disappearing)
Check that all the above work on mobile.
If you have a single-site account, test everything works on that too.
Check all the browsers we support (Chrome, Safari, Firefox, Edge, IE), mobile ones too (Safari, Chrome)!

Please take extra care in testing this one since it's a very important piece of the navigation.

Original Text:

Old:

New (24px):

artpi · 2015-11-25T10:29:30Z

Looks good so far, works good also

folletto · 2015-11-27T12:54:45Z

Tested on Chrome, Firefox, Safari.

The icon seems slightly lower than the text, fixed that, works. :)

seear · 2015-12-07T18:31:36Z

Logged-out
Old:

New:

The WP gridicon is very slightly lower logged-out than logged-in for a reason that I cannot yet ascertain.

folletto · 2015-12-08T18:22:17Z

I've to wrap up for the day, but so far I think I managed to:

Refactored the React components to use just one, MasterbarItem.
Heavily restructured the DOM (from a 7-levels hierarchy to a 2-levels one).
Cleared the CSS as a consequence, making it with flex.

Which sorted out automatically all the alignment issues.

Missing:

Hooking up SitesPopover.
Hooking up Notifications.
Cross-browser / device testing / debugging (I'm already aware of a couple of things: colors and avatar).
Testing in general (it's a very important component, so I'd double down).
Cleanup of the then unused code and components.

rickybanister · 2015-12-08T21:59:35Z

Yay for flex.

The spacing between things on the right side bothered me with the old layout, but it seems even worse with the new layout:

I realize it's different if you hover, but it just looks wrong most of the time—any thoughts on what we might do?

folletto · 2015-12-08T22:44:17Z

Yeah as you noticed right now it's purely, mathematically, padded. We can do optical adjustment before merging once the core of this PR is done. The way the code is structured now should make it piece of cake. :)

folletto · 2015-12-11T12:31:26Z

The SitesPopover is now hooked up in a new component MasterbarItemNew.

folletto · 2015-12-11T13:23:31Z

First step in componentizing Notifications with MasterbarItemNotifications. Right now just toggles.

Missing:

Set active state when open
Show notification dot

folletto · 2015-12-15T13:03:44Z

Tested on Chrome, Firefox, Safari, IE11, Edge. Works. 👍

With the changes above now the notification dot works, and the style issues across the browsers all work.

There are still two bugs:

On Firefox 42, the first click on the Notification icon doesn't open the panel. Further clicks do.
Blocker.
When the Notification area is clicked, the other highlight doesn't turn off.
Non-blocker.

And some style changes, as noted above:

Spacing between icons on the right.

jasmussen · 2015-12-15T13:15:27Z

A couple of quick observations just tested in Chrome today at various viewport sizes:

1: New post button doesn't have white fill when active, like current masterbar has:

2: Loading "pulse dot" overlays "new post" button in responsive mode. Not actually sure if this is unique to the new masterbar or if this also affects the old:

3: In a viewport width that invoke the mobile breakpoint, I couldn't get notifications to work at all. The button would be blue indicating it was clicked, but nothing would happen:

I will test more later on, using devices and other browsers.

folletto · 2015-12-15T13:20:43Z

Thanks!

1 — fixed, pushed a commit.
2 — it affects the old too, but since this is flexing it can be bigger than the cover. Reviewing...
3 — ach. definitely a blocker.

So, summary of outstanding issues so far for ease of reading:

On mobile the Notification panel doesn't show.
Blocker.
On Firefox 42, the first click on the Notification icon doesn't open the panel. Further clicks do.
Blocker.
Loader dot covering doesn't cover enough.
Blocker.
When the Notification area is clicked, the other highlight doesn't turn off.
Non-blocker.
Spacing between icons on the right should be tightened.
Non-blocker.

folletto · 2015-12-15T15:07:22Z

(1) On mobile the Notification panel doesn't show.
Blocker.

Fixed.

(2) On Firefox 42, the first click on the Notification icon doesn't open the panel. Further clicks do.
Blocker.

I discovered this is a bug present in production too. Not a blocker anymore for this PR. Opened an issue for it on #1611.

aduth · 2015-12-15T15:08:41Z

client/layout/masterbar-item-notifications.jsx

+
+	getInitialState() {
+		let newNote = false;
+		let	user;


Accidental tab here after let should be space.

folletto · 2015-12-15T15:28:58Z

Found another bug: dot bubble is misaligned on mobile.

folletto · 2015-12-15T16:05:02Z

(3) Loader dot covering doesn't cover enough.
Blocker.

Fixed.
Also made the background appear only on mobile sizes.

mtias · 2015-12-15T16:06:59Z

Looking nice!

apeatling · 2015-12-15T16:12:31Z

Great updates so far. Still not convinced about the no-label usability impact on mobile (even though visually it looks better) but I'm not sure how we'd measure that.

Also -- I mentioned it in the previous issue -- this should not merge until the front end masterbar matches. Otherwise we run the risk of that being forgotten and having a huge lag.

folletto · 2015-12-15T16:16:33Z

Great updates so far. Still not convinced about the no label usability impact on mobile (even though visually it looks better) but I'm not sure how we'd measure that.

That's mostly a byproduct of the cleanup code. I thought to leave that in and see if we need to add them back in. At this stage, I'd merge it like this and revert later if we think it's better. But open to discussion.

Also -- I mentioned it in the previous issue -- this should not merge until the front end masterbar matches. Otherwise we run the risk of that being forgotten and having a huge lag.

True. This however goes far far far beyond the change of icons. So I feel it's rather important we push this as soon as it's ready — whatever this means.

Cleanup: named z-index lost in rebase Identation fix Remove dead imports

Fix: force correct icon color on Firefox and IE11. Fix: active state on Editor Fix: show Notification panel showing in mobile breakpoint. Fix: spacing Fix: layout loader. Fix: align dot bubble position on mobile Fix: better visual rhythm for rightmost icons. Fix: active Notifications now disable other active states

remove masterbar-new-post remove masterbar-logged-out-menu

`section` gets initially passed as `false`, then becomes a string. Fix: hooked up Editor preload Cleanup: null and 0 are equivalent for ">" operator. Cleanup: made user "isRequired". Cleanup: shorter unseen notes code Fix whitespace Cleanup: isActive Cleanup: using ES6 default param Fix: the bar (fixed) now doesn't scroll away on mobile Fixes #1283

- Create "masterbar" folder. - Move sub-components. - Clean up layout folder. - Consolidate styles in the component. Remove old assets/stylesheet file for masterbar.

Masterbar: clean up, component refactor, and move to gridicons

aduth · 2015-12-21T19:25:12Z

Was it intentional to not use the <SiteStatsStickyLink /> component for the My Sites link in the master bar? Now when I click My Sites, I'm always directed to the "All Sites" view. For reference, the code clearly links to the /stats base URL:

https://github.com/Automattic/wp-calypso/blob/d84201f/client/layout/masterbar/index.jsx#L69

folletto · 2015-12-21T23:55:29Z

Yes and No :)

Yes: the masterbar was a hairball of very different components, each with different use and intent, so it has been normalized using all the same component.

No: that logic bit was missed since it didn't appear as a branching function on the URL logic, but stacked as component.

I'd re-add that at the masterbar level (which handles the logic), as a simple function.

folletto · 2015-12-22T00:03:41Z

...or as you did in #1894 heh :)

lancewillett · 2015-12-22T03:17:46Z

Looks like this change broke the n keyboard shortcut to open Notifications.

TypeError: Failed to execute 'contains' on 'Node': parameter 1 is not of type 'Node'. in Chrome console
client/layout/masterbar/notifications.jsx:67

Logged bug in #1907

apeatling · 2015-12-30T19:17:04Z

@seear What's the plan to update the black front-end masterbar with the same changes? I was under the impression this would be pretty soon after, but I'm still seeing the labeled version.

folletto · 2016-01-04T10:25:19Z

It has been updated to Gridicons, the labels were kept mostly for ease of the first patch. We should align the labels off too now, with a follow-up patch on front-end.

apeatling · 2016-01-04T15:39:57Z

👍

seear · 2016-01-05T16:24:03Z

@apeatling: frontend masterbar is now updated to match Calypso label/breakpoint behaviour.

apeatling · 2016-01-05T20:02:29Z

🎉

mtias added [Status] In Progress Masterbar [Type] Janitorial labels Nov 24, 2015

sendhil mentioned this pull request Dec 3, 2015

Update Tab Bar Icons to Match Latest on Web wordpress-mobile/WordPress-iOS#4546

Closed

ehg mentioned this pull request Dec 7, 2015

Masterbar: Refactor and isolate logged out functionality #1326

Closed

3 tasks

folletto mentioned this pull request Dec 8, 2015

Layout: while viewing sidebar the masterbar allows you to erroneously scroll the main content column #1387

Closed

folletto mentioned this pull request Dec 8, 2015

Masterbar: mobile masterbar doesn't stick when scrolling #1283

Closed

mtias force-pushed the update/masterbar branch from ffbdec0 to 318704c Compare December 9, 2015 12:19

lancewillett removed the Design label Dec 9, 2015

folletto mentioned this pull request Dec 9, 2015

Masterbar: remove labels on mobile #1416

Closed

ehg force-pushed the update/masterbar branch from d1f883e to a698d7c Compare December 14, 2015 18:52

aduth reviewed Dec 15, 2015
View reviewed changes

ehg and others added 8 commits December 21, 2015 17:23

ES6ify components.

937afe2

Cleanup: named z-index lost in rebase Identation fix Remove dead imports

Cleanup: renamed variables, single classes call

564a932

Layout: less files.

aad188e

remove masterbar-new-post remove masterbar-logged-out-menu

Fix: removed padding: made the buttons not center aligned

854c67d

Masterbar: setup component organisation and style file.

b1c1233

- Create "masterbar" folder. - Move sub-components. - Clean up layout folder. - Consolidate styles in the component. Remove old assets/stylesheet file for masterbar.

Fix: tooltips added back + moved top level

d84201f

ehg force-pushed the update/masterbar branch from 813e083 to d84201f Compare December 21, 2015 17:25

seear added [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Dec 21, 2015

seear added a commit that referenced this pull request Dec 21, 2015

Merge pull request #631 from Automattic/update/masterbar

b2d9c21

Masterbar: clean up, component refactor, and move to gridicons

seear merged commit b2d9c21 into master Dec 21, 2015

seear deleted the update/masterbar branch December 21, 2015 17:36

scruffian removed the [Status] Ready to Merge label Dec 21, 2015

aduth mentioned this pull request Dec 21, 2015

Masterbar: Reintroduce sticky site logic to stats item #1894

Merged

lancewillett mentioned this pull request Dec 22, 2015

Notifications: can't open panel with n keyboard shortcut key after recent Masterbar refactor #1907

Closed

nylen mentioned this pull request Jan 4, 2016

Site Picker: Scroll to selected site when opening picker #1641

Merged

lancewillett added Navigation and removed Navigation labels Feb 6, 2017

folletto self-assigned this Feb 28, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Masterbar: clean up, component refactor, and move to gridicons #631

Masterbar: clean up, component refactor, and move to gridicons #631

mtias commented Nov 24, 2015

artpi commented Nov 25, 2015

folletto commented Nov 27, 2015

seear commented Dec 7, 2015

folletto commented Dec 8, 2015

rickybanister commented Dec 8, 2015

folletto commented Dec 8, 2015

folletto commented Dec 11, 2015

folletto commented Dec 11, 2015

folletto commented Dec 15, 2015

jasmussen commented Dec 15, 2015

folletto commented Dec 15, 2015

folletto commented Dec 15, 2015

aduth Dec 15, 2015

folletto commented Dec 15, 2015

folletto commented Dec 15, 2015

mtias commented Dec 15, 2015

apeatling commented Dec 15, 2015

folletto commented Dec 15, 2015

aduth commented Dec 21, 2015

folletto commented Dec 21, 2015

folletto commented Dec 22, 2015

lancewillett commented Dec 22, 2015

apeatling commented Dec 30, 2015

folletto commented Jan 4, 2016

apeatling commented Jan 4, 2016

seear commented Jan 5, 2016

apeatling commented Jan 5, 2016

Masterbar: clean up, component refactor, and move to gridicons #631

Masterbar: clean up, component refactor, and move to gridicons #631

Conversation

mtias commented Nov 24, 2015

Original Text:

artpi commented Nov 25, 2015

folletto commented Nov 27, 2015

seear commented Dec 7, 2015

folletto commented Dec 8, 2015

rickybanister commented Dec 8, 2015

folletto commented Dec 8, 2015

folletto commented Dec 11, 2015

folletto commented Dec 11, 2015

folletto commented Dec 15, 2015

jasmussen commented Dec 15, 2015

folletto commented Dec 15, 2015

folletto commented Dec 15, 2015

aduth Dec 15, 2015

Choose a reason for hiding this comment

folletto commented Dec 15, 2015

folletto commented Dec 15, 2015

mtias commented Dec 15, 2015

apeatling commented Dec 15, 2015

folletto commented Dec 15, 2015

aduth commented Dec 21, 2015

folletto commented Dec 21, 2015

folletto commented Dec 22, 2015

lancewillett commented Dec 22, 2015

apeatling commented Dec 30, 2015

folletto commented Jan 4, 2016

apeatling commented Jan 4, 2016

seear commented Jan 5, 2016

apeatling commented Jan 5, 2016