1054274 - Split out browser overflow menu into a shared component

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::System::Browser Chrome, defect)

Description

[Chris Lord [:cwiiis]]

UX want the messaging app to use the same overflow menu as the browser. Let's split it out into a shared component to enable that.

Comment 1

[Chris Lord [:cwiiis]]

Attachment: Split browser overflow menu into shared component

This splits the browser overflow menu into a shared component. At the same time, it:

- Simplifies the structure of the overflow menu (removed lots of extraneous divs)
- Adds the menu highlight colour (per spec)
- Lazy-loads the Bookmarks database (a review suggestion from the original landing that I didn't get to)

Comment 2

[Ben Francis [:benfrancis]]

Comment on attachment 8473673 [details] [review]
Split browser overflow menu into shared component

Sorry it took me ages to get to this and it has bitrotted.

It looks pretty cool. Wilson, do you have time to review this from the point of view of web components? This is all new to me.

Comment 3

[Wilson Page [:wilsonpage]]

Comment on attachment 8473673 [details] [review]
Split browser overflow menu into shared component

Look like a great first pass! Coupla things need addressing (see Github) before I'll r+.

Most important is that <gaia-overflow-menu-option> is probably not required. Instead we could be more flexible and allow the user to use any child element. This would be fewer custom elements, fewer style-sheets and fewer shadow-dom instances for the Browser (and us) to worry about.

Comment 4

[Chris Lord [:cwiiis]]

Comment on attachment 8473673 [details] [review]
Split browser overflow menu into shared component

Addressed review comments (those I didn't I've commented on on github, but the major ones have been addressed I think)

Comment 5

[Wilson Page [:wilsonpage]]

Comment on attachment 8473673 [details] [review]
Split browser overflow menu into shared component

Couple of nits (see pull-request), but looks good to me. Converted to f+ as I'm not qualified to give r+ to work on this app :)

The biggest thing to me was the common use of `*` in the CSS. If anything looks out of the ordinary I'd like a comment to help me understand.

Comment 6

[Chris Lord [:cwiiis]]

Comment on attachment 8473673 [details] [review]
Split browser overflow menu into shared component

Given Wilson's comment, can you review this Kevin?

Comment 7

[Kevin Grandon :kgrandon]

Comment on attachment 8473673 [details] [review]
Split browser overflow menu into shared component

I took a quick look at the code and it seems fine to me, nice work.

How do you suggest that we land this given the pending overflow menu uglification? Should we land this first? If not, this will probably be bitrotted, though it should be fine to land the component regardless.

Comment 8

[Dale Harvey (:daleharvey)]

It shouldnt bitrot me, fine with this being kept around and can hopefully be used as a base to update the dialog system wide

Comment 9

[Chris Lord [:cwiiis]]

Merged: https://github.com/mozilla-b2g/gaia/commit/a91973e69fe14e31510bc2c903b6d2b04f9fb228

There is some amount of dissonance in app_chrome.js, but no more than there already was. We'll clean this up in 2.2.