Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add context menu #2001

Merged
merged 15 commits into from Nov 3, 2017
Merged

Add context menu #2001

merged 15 commits into from Nov 3, 2017

Conversation

@rickycodes
Copy link
Contributor

@rickycodes rickycodes commented Jul 13, 2017

closes: #265

First stab at creating a right-click context menu...

right-click-copy

/cc @leo

@rickycodes rickycodes force-pushed the rickycodes:issue-265 branch from a80d48e to 1ba79c5 Jul 14, 2017
@rickycodes rickycodes changed the title Add context menu (WIP) Add context menu Jul 14, 2017
@timothyis timothyis requested a review from chabou Jul 14, 2017
@rickycodes
Copy link
Contributor Author

@rickycodes rickycodes commented Jul 15, 2017

Looking at the original issue, I saw #265 (comment) and decided to add two items to the context menu:

  • 'Open Selection as URL'
  • 'Search the Web for Selection'

selectionMenu

/cc @CodeTheory

@rickycodes rickycodes force-pushed the rickycodes:issue-265 branch 2 times, most recently from c91943e to 34b4738 Jul 15, 2017
@timothyis timothyis requested a review from matheuss Jul 15, 2017
@timothyis
Copy link
Contributor

@timothyis timothyis commented Jul 15, 2017

This is incredible! Can't wait to have this in! 🎉
cc @matheuss @rauchg @chabou @ppot

@rickycodes rickycodes force-pushed the rickycodes:issue-265 branch from 82337d2 to 725ae4b Jul 15, 2017
@rickycodes
Copy link
Contributor Author

@rickycodes rickycodes commented Jul 15, 2017

I ended up just using .concat to add the two new items to the end of the menu since splice is problematic (modifies the original) 😓

screenshot for context:
screen shot 2017-07-15 at 12 43 00 pm

@rickycodes rickycodes force-pushed the rickycodes:issue-265 branch from 7b8512a to d1d41dd Jul 15, 2017
@henrikdahl henrikdahl self-requested a review Jul 16, 2017
@Stanzilla
Copy link
Collaborator

@Stanzilla Stanzilla commented Jul 16, 2017

This does and will clash with

// if true, on right click selected text will be copied or pasted if no
    // selection is present (true by default on Windows)
    quickEdit: true,

so I guess having it enabled should disable the menu or the other way round

@Stanzilla
Copy link
Collaborator

@Stanzilla Stanzilla commented Jul 16, 2017

Same with

    // set to `true` (without backticks) if you're using a Linux setup that doesn't show native menus
    // default: `false` on Linux, `true` on Windows (ignored on macOS)
    showHamburgerMenu: '',

in the case of Windows or true the right click menu should probably adjust itself to not duplicate items with the hamburger menu

clear buffer command works but is still bugged on Windows (#1808 (comment))
cut undo select all and redo do not work. nothing happens.

@rickycodes
Copy link
Contributor Author

@rickycodes rickycodes commented Jul 16, 2017

cut undo select all and redo do not work. nothing happens.

do these items behave as expected or differently when using the main menus? (I haven't really been able to get them to work in either case), they should perform the same as the other menus (since they're just duplicated from there)

re: windows settings, I will tweak what I have to make sure there's no conflicts (and test a bit in windows) to see what makes the most sense.

/cc @Stanzilla

@Stanzilla
Copy link
Collaborator

@Stanzilla Stanzilla commented Jul 16, 2017

Yeah, they don't work from the main menu either. Do they work on any OS?

enabled: !isCollapsed
},
{
label: 'Search the Web for Selection',

This comment has been minimized.

@henrikdahl

henrikdahl Jul 16, 2017
Collaborator

Search with Google is more specific and consistent with other context menus ✌️

This comment has been minimized.

@timothyis

timothyis Jul 16, 2017
Contributor

Is there anyway to open a link with the users default search engine? That would be awesome.
Default search engine probably would come from the default browser > default search engine

This comment has been minimized.

@rickycodes

rickycodes Jul 16, 2017
Author Contributor

Could leave it as Google by default and make it configurable via .hyper.js?

This comment has been minimized.

@rickycodes

rickycodes Jul 16, 2017
Author Contributor

both terminal and iTerm2 have Search with Google

This comment has been minimized.

@timothyis

timothyis Jul 17, 2017
Contributor

Yeah, I'd like that. I know a few people who'd like not to use Google.

This comment has been minimized.

@henrikdahl

henrikdahl Jul 17, 2017
Collaborator

With Mac OS services you should be able to search with default browser and engine but most likely unusable since Hyper also supports other operative systems ✌️

This comment has been minimized.

@rickycodes

rickycodes Jul 17, 2017
Author Contributor

in b3a9d30 I've made this configurable via exposing a new key/value (searchEngineUrl) from config so this can be customized via .hyper.js

defaults to google, but can be anything the user desires.

this would likely need to be addressed in app/config/config-default.js as well (similar to bellSoundURL)

I left the wording as is.

/cc @henrikdahl @CodeTheory

@rickycodes
Copy link
Contributor Author

@rickycodes rickycodes commented Jul 16, 2017

Yeah, they don't work from the main menu either. Do they work on any OS?

this is a good question, I'm not sure under what circumstances they are designed to work... the main thing is that this is not a regression on my part 😉

@rickycodes rickycodes force-pushed the rickycodes:issue-265 branch 2 times, most recently from b3a9d30 to 70f4aec Jul 17, 2017
@rickycodes
Copy link
Contributor Author

@rickycodes rickycodes commented Jul 22, 2017

I nixed the url/google functionality (as I think that should be handled in a different PR, or possibly a plugin?)

@Stanzilla this should account for the items you mentioned quickEdit and showHamburgerMenu. let me know if this makes sense

/cc @CodeTheory

@Stanzilla
Copy link
Collaborator

@Stanzilla Stanzilla commented Jul 22, 2017

showHamburgerMenu is a bit problematic, the default is '' which is sort of auto, that means it is actually true on Windows even if it is not set to true in the config. That is a problem because a Windows user can't set it to falsewhen he would prefer using your context menu over the hamburger and also makes you checking for it kinda useless since it will never be false on Windows.

My concern was to not duplicate menu entries when both hamburger and context are enabled but since you removed the URL/Google option it's 100% the same anyway, no?

@rickycodes
Copy link
Contributor Author

@rickycodes rickycodes commented Jul 22, 2017

@Stanzilla what do you think makes the most sense in your opinion? should I just drop the showHamburgerMenu check? I'm not entirely sure what makes the most sense for this as I am not a windows user

maybe I'll fire up a windows VM and see 👁️

@Stanzilla
Copy link
Collaborator

@Stanzilla Stanzilla commented Jul 22, 2017

I'd drop the check, yeah. Makes the most sense to me.

@rickycodes rickycodes force-pushed the rickycodes:issue-265 branch from 811585d to 742d7c9 Jul 22, 2017
@rickycodes
Copy link
Contributor Author

@rickycodes rickycodes commented Jul 29, 2017

@Stanzilla I dropped that showHamburgerMenu

lmk if this makes sense

@ppot
Copy link
Collaborator

@ppot ppot commented Aug 2, 2017

@rickycodes Are you binding the menu context with keymaps?

@rickycodes
Copy link
Contributor Author

@rickycodes rickycodes commented Aug 2, 2017

@ppot I'm just building the context menu from what already exists (combining edit + shell .submenu's here: https://github.com/zeit/hyper/pull/2001/files#diff-ac33cd5e16269e3f364363f9865b9ff5R24)

so yea, they have the same role/accelerator properties, if that's what you mean?

@rickycodes rickycodes force-pushed the rickycodes:issue-265 branch from bb9baa6 to 4128d51 Oct 13, 2017
@rickycodes
Copy link
Contributor Author

@rickycodes rickycodes commented Oct 13, 2017

@chabou this has now been rebased over zeit:canary and is now targeting it

I had to make a couple adjustments to accommodate the new changes

lmk what you think


const contextMenuTemplate = createWindow => {
return editMenu(commands).submenu.concat({type: 'separator'}, shellMenu(commands, createWindow).submenu);
};

This comment has been minimized.

@rickycodes

rickycodes Oct 14, 2017
Author Contributor

maybe an alternative to this:

const contextMenuTemplate = createWindow => {
  const shell = shellMenu(commands, createWindow).submenu
  const edit = editMenu(commands).submenu
  return edit.concat({type: 'separator'}, shell);
};
@rickycodes
Copy link
Contributor Author

@rickycodes rickycodes commented Oct 20, 2017

@chabou have you had a chance to look at this? I've actually made some refinements, and have an example where the menu changes/updates based on selection... in this version I've made it so cut and copy are only available if text is selected... text selection also expands two new options: Open Selection as URL and Search the Web for Selection:

hyper

you can compare the changes here: rickycodes/hyper@rickycodes:issue-265...add-context-menu

lmk what you think!

Copy link
Collaborator

@chabou chabou left a comment

Please remove all right click mentions

@@ -64,6 +64,13 @@ export default class Terms extends Component {
this.terms[uid] = term;
}

componentDidMount() {
window.addEventListener('contextmenu', () => {

This comment has been minimized.

@chabou

chabou Oct 29, 2017
Collaborator

I didn't know/see there was this event.
In this case, please remove any mention of right click notion. We should only speak about context menu.

@chabou
Copy link
Collaborator

@chabou chabou commented Oct 29, 2017

I love the idea to pass selection in redux action.
Please make it happen 🙏

@rickycodes
Copy link
Contributor Author

@rickycodes rickycodes commented Oct 30, 2017

@chabou I've made the requested changes

@chabou
Copy link
Collaborator

@chabou chabou commented Oct 30, 2017

I have a problem with selection with your PR.
When I right click to show contextMenu at a point "P", when I discard contextMenu, xterm is on SelectionMode starting on this point "P" (but I haven't any mouse button pressed).
See:
contextmenu

@chabou
Copy link
Collaborator

@chabou chabou commented Oct 30, 2017

Can you remove "Open Selection as URL" and "Search the webfor Selection"? I don't think it should be a core feature. But it is perfect to filter Copy/Cut in contextMenu depending on selection presence.

@rickycodes
Copy link
Contributor Author

@rickycodes rickycodes commented Oct 30, 2017

@chabou I've removed the extended menu with the additional options. I noticed canary has the ability to click on URLs anyway, so maybe Open Selection as URL isn't so useful now

we can discuss creating plugins to accomplish those items once this lands/how to expand on this so plugin authors can modify the context menu

@Stanzilla
Copy link
Collaborator

@Stanzilla Stanzilla commented Oct 30, 2017

The CircleCI failure looks unrelated.

@rickycodes
Copy link
Contributor Author

@rickycodes rickycodes commented Oct 30, 2017

@Stanzilla yea i noticed that, but it seems unrelated (the build was passing earlier and I haven't made any adjustments that would have changed that)

@chabou
Copy link
Collaborator

@chabou chabou commented Nov 3, 2017

I can't reproduce #2001 (comment) anymore 🤔

Let's merge this. We'll fix bugs if needed: not a big deal.

@chabou
Copy link
Collaborator

@chabou chabou commented Nov 3, 2017

@rickycodes I can't push to your branch. Did you allow maintainers to modify your branch?
I resolved conflicts (in GitHub interface) and Github doesn't see my last commit 😞.
Can you push a empty commit to trigger it ?

@rauchg
Copy link
Contributor

@rauchg rauchg commented Nov 3, 2017

This feature is 🔥

@chabou
chabou approved these changes Nov 3, 2017
@chabou chabou merged commit 62e29ef into vercel:canary Nov 3, 2017
3 checks passed
3 checks passed
ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@chabou
Copy link
Collaborator

@chabou chabou commented Nov 3, 2017

Thank you so much for your help and your awesome work 🙏
Really sorry for the multiple delays along this PR.

@rickycodes rickycodes deleted the rickycodes:issue-265 branch Jan 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

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