Skip to content
This repository has been archived by the owner on Nov 1, 2022. It is now read-only.

Custom Tab: Add ability to add menu items #1398

Closed
pocmo opened this issue Nov 16, 2018 · 2 comments
Closed

Custom Tab: Add ability to add menu items #1398

pocmo opened this issue Nov 16, 2018 · 2 comments
Assignees
Labels
<customtabs> Component: feature-customtabs 🌟 feature New functionality and improvements <menu> Component: concept-menu, browser-menu, browser-menu2 🌐 reference browser Features, bugs, issues related to the reference browser implementation <toolbar> Components: browser-toolbar, concept-toolbar
Milestone

Comments

@pocmo
Copy link
Contributor

pocmo commented Nov 16, 2018

User story: mozilla-mobile/reference-browser#209

CustomTabsIntent.Builder.addMenuItem()

This may require changes to our menu/toolbar component too.

┆Issue is synchronized with this Jira Task

@pocmo pocmo added 🌟 feature New functionality and improvements <toolbar> Components: browser-toolbar, concept-toolbar <menu> Component: concept-menu, browser-menu, browser-menu2 🌐 reference browser Features, bugs, issues related to the reference browser implementation L <customtabs> Component: feature-customtabs labels Nov 16, 2018
@jonalmeida
Copy link
Contributor

This may require changes to our menu/toolbar component too.

It does! 😄

Simply adding menu items to the menu builder persists them for the lifetime of the application since the menu builder lives in the Components. My naive implementation that I have is to hold on to a list of menu items we add to the menu, and in stop we filter out the menu items and pass it back to the menu builder.

So far it seems to work, but it doesn't feel right. I had an older patch that added a 'tag' attribute for menu items so that we could give a tag to the CT items and filter them out easily.

@pocmo would the latter solution work? I recall you wanted to do some work with adding ids to menu items, but I'm hoping this solution doesn't overlap.

@pocmo
Copy link
Contributor Author

pocmo commented Jan 22, 2019

Modifying the global configuration for the menu sounds dangerous and may introduces weird races while switching between custom tabs and the actual browser screen.

I'd prefer if we either have two builders (one for the normal menu) and one for custom tabs (that gets build dynamically with CustomTabConfig as input) OR a way to build on top of the existing builder which basically creates a copy and attaches some additional items (e.g. menuBuilder = components.menuBuilder + something(CustomTabConfig)).

Currently we assign the menu builder to the toolbar ourselves (in BrowserFragment). Maybe this could happen inside a feature and then the feature can decide what to do.

@jonalmeida jonalmeida added this to the 0.40 🎧 milestone Jan 22, 2019
jonalmeida added a commit to jonalmeida/android-components that referenced this issue Jan 22, 2019
@pocmo pocmo mentioned this issue Jan 24, 2019
12 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
<customtabs> Component: feature-customtabs 🌟 feature New functionality and improvements <menu> Component: concept-menu, browser-menu, browser-menu2 🌐 reference browser Features, bugs, issues related to the reference browser implementation <toolbar> Components: browser-toolbar, concept-toolbar
Projects
None yet
Development

No branches or pull requests

2 participants