Skip to content
This repository has been archived by the owner. It is now read-only.

Cleanup the multi-type context menu item #8503

Merged
merged 1 commit into from May 2, 2017
Merged

Conversation

@ardcore
Copy link
Contributor

ardcore commented Apr 26, 2017

This fixes #7816 and simplifies the logic of context menu item by making a distinction between the submenu collection and the structure of a multi-type item.

By creating a new collection (items), we're able to treat the multi-item as a normal item (sharing all it's behaviours -- lack of them was the precise cause of #7816 ). This also solves some other, probably unfilled issues (i.e, it wasn't possible to focus the multi-type item using keyboard[1]), and makes it possible to define multi-type items with submenus, if needed.

[1] - While this PR makes it possible to select the multi-item, keyboard navigation still won't work inside it. I believe this should be discussed in another issue.

  • Submitted a ticket for my issue if one did not already exist.
  • Used Github auto-closing keywords in the commit message.
  • Added/updated tests for this change (for new code or code which already has tests).
  • Ran git rebase -i to squash commits (if needed).

Test Plan:

See #7816

Fix #7816
Copy link

cndouglas left a comment

Works for me.

Copy link
Member

bsclifton left a comment

Works great 😄

lint passes, unit tests pass, and webdrivers tests using npm run test -- --grep="hamburger" pass

@bsclifton bsclifton added this to the 0.15.2 milestone May 2, 2017
@bsclifton bsclifton merged commit e1e294c into brave:master May 2, 2017
0 of 2 checks passed
0 of 2 checks passed
continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
continuous-integration/appveyor/pr AppVeyor build failed
Details
@bsclifton
Copy link
Member

bsclifton commented May 2, 2017

@ardcore congrats on your 1st commit landing in Brave 🎉 😄 (hopefully of many more!). If you need help with anything in the future, please let me know

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants
You can’t perform that action at this time.