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

Make new tab context menu native #8397

Merged
merged 1 commit into from
Apr 20, 2017
Merged

Make new tab context menu native #8397

merged 1 commit into from
Apr 20, 2017

Conversation

cndouglas
Copy link

Fixes #7748

  • 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:

  1. Open enough tabs to cover the entire tabs bar.
  2. Right-click the new tab button (the "+" button).
  3. Mouse over the New Session Tab menu item.
  4. Make sure the new session submenu is onscreen.

Screenshot:
image

@bsclifton
Copy link
Member

@cezaraugusto has tested on macOS and I just tried on Windows- this works great! 😄 very very nice. Huge thanks for knocking this out so quick

Here's a screenshot, just for folks to see:
screen shot 2017-04-19 at 12 12 33 pm

Copy link
Contributor

@cezaraugusto cezaraugusto left a comment

Choose a reason for hiding this comment

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

tested on macOS, works great ++
There is a test failing after this change but I think @bsclifton have a good plan for that

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.

Awesome job! Only feedback I have is: this breaks a test which was depending on this being DOM selectable (via one of the webdriver tests).

Would you mark the failed tests with a .skip? Other than than, this looks great 😄 Thanks again!

@cezaraugusto
Copy link
Contributor

for reference of #8397 (review).

Link: https://github.com/brave/browser-laptop/blob/master/test/tab-components/tabTest.js#L105

shows a context menu when long pressed (click and hold)
shows a context menu when right clicked

@bsclifton
Copy link
Member

bsclifton commented Apr 19, 2017

Long term, it would be really nice to have the contextMenus.js code detect whether or not it's running in a test. If so, it could render the old way (creating a contextMenu object in React). This would be a big scale change though- for now, marking the failed test with .skip would work 😄

Now that we have a proposed solution for async menus (still needs testing / integration) with brave/muon#161, we should be able to convert all the old style menus to native context menus. Since you've already done that before (ex: the reload button and a few other places, IIRC), if you wanted to create an issue to track what needs to be converted and then also which has skipped tests (like this one), that would be awesome 😄 I'm going to try my best to dig into the Muon PR above so that we can have async native menus

@cndouglas
Copy link
Author

I disabled the tests.

@bsclifton Great idea! I'll see what I can do.

@cezaraugusto cezaraugusto merged commit 0e9b8e0 into brave:master Apr 20, 2017
@cndouglas cndouglas deleted the newtab-context-menu branch April 20, 2017 14:58
@bbondy
Copy link
Member

bbondy commented Apr 21, 2017

💯

@@ -102,14 +102,14 @@ describe('tab tests', function () {
.click(newFrameButton)
.waitForExist('[data-test-id="tab"][data-frame-key="2"]')
})
it('shows a context menu when long pressed (click and hold)', function * () {
it.skip('shows a context menu when long pressed (click and hold)', function * () {
Copy link
Contributor

Choose a reason for hiding this comment

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

@liunkae Was this added to the tracking ticket?

Copy link
Author

Choose a reason for hiding this comment

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

@luixxiul Thanks! Added.

bsclifton pushed a commit that referenced this pull request Apr 25, 2017
@bsclifton
Copy link
Member

Change is small- I pulled this into 0.15.0 per recommendation by @bradleyrichter

@bsclifton bsclifton modified the milestones: 0.15.0, 0.15.1 Apr 25, 2017
@bsclifton
Copy link
Member

Moving back to 0.15.1. There are some other changes which this relies on. Unfortunately, pulling in is not as easy as I thought ☹️

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.

5 participants