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

Add context menu to reload button #5802

Merged
merged 1 commit into from
Dec 4, 2016
Merged

Add context menu to reload button #5802

merged 1 commit into from
Dec 4, 2016

Conversation

cndouglas
Copy link

@cndouglas cndouglas commented Nov 23, 2016

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

Fixes #5796

This enhancement takes advantage of LongPressButton, which has long press and right-click functionality out of the box. The context menu contains Reload Page and Clean Reload, the reload options currently implemented in Brave (from the View menu). Thus, Reload Page and Clean Reload have been moved to CommonMenu.

Test Plan:

  1. Open a new Brave window.
  2. Navigate to any site.
  3. Right-click the reload button in the urlBar.
  4. Make sure the context menu appears.
  5. Click Reload Page.
  6. Make sure the page is reloaded.
  7. Right-click the reload button again.
  8. Click Clean Reload.
  9. Make sure the page is reloaded clean.
  10. Long press the reload button.
  11. Make sure the context menu also appears.

Screenshot (updated):
image

@bradleyrichter
Copy link
Contributor

++

@bsclifton
Copy link
Member

Woah! Great job with this ❤️ !!

@cndouglas
Copy link
Author

The context menu is now a native menu. New screenshot:
image

@bsclifton
Copy link
Member

@liunkae with a popup, this might cause an issue; what happens if you have a YouTube video playing and then the menu pops up? I think it'll freeze the app ☹️

@cndouglas
Copy link
Author

cndouglas commented Nov 23, 2016

@bsclifton Yes, it does freeze temporarily. But do you think the benefits (appearance, long press drag release, staying in screen bounds, etc.) of a native menu outweigh the downside (temporary freeze)?

@bsclifton
Copy link
Member

@bbondy @bradleyrichter what do you think of the above ^^ ?

@cndouglas
Copy link
Author

By the way, also using a native menu for the new tab dropdown could fix #5763.
image

@cndouglas
Copy link
Author

Just in case, I'll leave the code to change back to non-native menus:
In contextMenus.js:

function onReloadContextMenu (target) {
  const rect = target.getBoundingClientRect()
  const menuTemplate = [
    CommonMenu.reloadPageMenuItem(),
    CommonMenu.cleanReloadMenuItem()
  ]

  windowActions.setContextMenuDetail(Immutable.fromJS({
    left: rect.left,
    top: rect.bottom + 2,
    template: menuTemplate
  }))
}

@bsclifton
Copy link
Member

@liunkae if you can change it back to a non-native menu for now and then rebase, that would be awesome 😄

We definitely appreciate the work and the native version does look good- but we'll want to keep the non-native version until we can solve (or work around) the screen blocking during popup

@bsclifton bsclifton added this to the 0.13.0 milestone Dec 3, 2016
@bsclifton bsclifton self-assigned this Dec 3, 2016
@bsclifton
Copy link
Member

bsclifton commented Dec 3, 2016

One advantage of the non-native menus (and feel free to take advantage of this):
You can test them using webdriver 😄 If you wanted to see a similar test case, you should be able to find long press test cases for the back/forward buttons and also the new tab + (if you wanted to add one for this too)

@cndouglas
Copy link
Author

@bsclifton I changed back to a non-native menu and fixed the file conflicts.

Sorry for asking, but how do I rebase with the Brave master branch (since this branch is in my fork)? I searched everywhere, but I just cannot figure it out…

@luixxiul
Copy link
Contributor

luixxiul commented Dec 4, 2016

git checkout reload-dropdown
git fetch upstream
git rebase upstream/master

and follow the instruction :-)

@bsclifton
Copy link
Member

@liunkae no worries- I just rebased it for you! Let me give it one last test, then I'll merge. Thanks again 😄

@bsclifton
Copy link
Member

Tested on macOS and Windows, looking great! 👍

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.

4 participants