Skip to content
This repository has been archived by the owner on Dec 11, 2019. It is now read-only.

Refactors MenuBar and MenuBarItem into redux component #8656

Merged
merged 1 commit into from
May 4, 2017

Conversation

NejcZdovc
Copy link
Contributor

@NejcZdovc NejcZdovc commented May 3, 2017

Test Plan

  • open menu on windows
  • try clicking around
  • everything should behave as did before

Description

  • 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).

Resolves #8652

Auditors: @bsclifton @bridiver

@NejcZdovc NejcZdovc added this to the 0.15.2 milestone May 3, 2017
@NejcZdovc NejcZdovc self-assigned this May 3, 2017
@NejcZdovc NejcZdovc requested review from bridiver and bsclifton May 3, 2017 10:12
@NejcZdovc NejcZdovc mentioned this pull request May 3, 2017
51 tasks
@NejcZdovc NejcZdovc force-pushed the redux/menubar branch 3 times, most recently from b8c397f to 7dcb919 Compare May 3, 2017 18:59
Copy link
Member

@bsclifton bsclifton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code changes look good- some notes as I'm testing on Windows:

  • when "Hide the menu bar by default" is true, the menus will automatically open (without clicking), which is not correct. Before, it would require a click to open the menu

That seems to be the only thing I can find 👍

@NejcZdovc
Copy link
Contributor Author

@bsclifton fix added. Can you please retest it?

Resolves brave#8652

Auditors: @bsclifton @bridiver

Test Plan:
- open menu on windows
- try clicking around
- everything should behave as did before
Copy link
Member

@bsclifton bsclifton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-tested; works great! Thanks for sharing the diff

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

Successfully merging this pull request may close these issues.

3 participants