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

TinyMCE Improvements: Open links from note window with popup and auto-create links (for 4.0 branch) #450

Closed
wants to merge 1 commit into from

Conversation

jlegewie
Copy link
Contributor

@jlegewie jlegewie commented Feb 3, 2014

Okay, here is an updated version for the 4.0 branch.

  1. I'll accept this on the 4.0 branch for 4.0.18...

I have Moved the pull-request.

  1. We should pass through the modifier keys to loadURI so that people can open in a new tab/window, etc. You can see the properties checked by loadURI() (.shiftKey, etc.) and just pass a fake event object with those copied from the original event.

I tried but have problems doing this because I am unable to get the original event. Maybe you have an idea what's going on. Currently, the linksmenu plugin adds a command using ed.addCommand('openlink', which calls zoteroHandleEvent with a fake event (but without .shiftKey, etc.). This 'openlink' command is called when the user clicks the button based on this line cmd : 'openlink' in the definition of the Open Link button. The command is not an event though so that I don't know how to get the data. An alternative would be to replace cmd : 'openlink' with onclick: function() {... call to zoteroHandleEvent... but that function only gets the node as an argument not the event.

  1. Rather than modifying tiny_mce.js (which we generally want to avoid, since it makes upgrades more annoying), can't you just add the 'openlink' command in the setup function in note.html?
  1. Done. The command is now defined in the plugin so that the linksmenu plugin contains all the relevant code. There is, however, a second place were I used to modify tinymce code. I changed the contextmenu plugin to add the open button to the right click menu as well. It's a little annoying because the contextmenu plugin is minified so that my small change replaces the entire plugin. I don't think there is a workaround for that. This pull request does not include this commit for now. I can add the commit, I could create a new plugin contextmenu2 with the modification, or the open link button does not appear in the right-click menu but I think it's nice. What is your preference?
  1. I'm seeing a little glitchiness...

For me, two finger click on touchpad is right click (OSX/Firefox) and I can't reproduce the problem. So it would be great if you can take a look.

  1. I'd say that, with the single-click menu, we should get rid of the Unlink button in the toolbar and just leave the one link button.

I remove the Unlink button from the toolbar.

  1. There is another limitation. The current implementation does not work for separate note windows. It's probably easy to fix but I wasn't sure how. The popup appear but clicking on "open link" doesn't do anything. I think the problem is in loadURI, which might try to open the link in the separate note window.

@@ -138,6 +138,9 @@

case 'change':
break;

case 'openlink':
ZoteroPane.loadURI(event.target.href);
Copy link
Member

Choose a reason for hiding this comment

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

Indent and add "break;"

@jlegewie
Copy link
Contributor Author

jlegewie commented Feb 4, 2014

no idea what was going on with the indent but it's fixed.

@dstillman
Copy link
Member

I'll look into passing through the modifier keys and getting this to work in separate note windows.

For contextmenu, it looks like this will be easier once we upgrade to TinyMCE 4, which lets you define menu items that can then be added to the menu. In the meantime, we can just include the non-minified version from GitHub — I assume it's just this — and add a comment that says "// Added by Zotero".

For me, two finger click on touchpad is right click (OSX/Firefox) and I can't reproduce the problem. So it would be great if you can take a look.

I can look into it.

Otherwise, this looks good. You can squash these commits together into a single one for merging.

@jlegewie
Copy link
Contributor Author

jlegewie commented Feb 4, 2014

I replaced the contextmenu plugin with the full version and added the open link button (only appears when the user clicks on links). Commits are squashed...

@dstillman
Copy link
Member

Sorry, could you just clean up that commit message a little and remove unnecessary stuff ("Indent correctly")? You can use git ci --amend to edit it and then force-push again.

Also, can you add a comment to the top of linksmenu/editor_plugin.js saying that it's based on the TinyMCE contextmenu plugin (which I think is what you said)?

@jlegewie
Copy link
Contributor Author

jlegewie commented Feb 4, 2014

Sorry, my git skills are still pretty bad. The message is fixed.

There already is a comment saying

* This plugin adds a left-click context menu to links in the TinyMCE editor for Zotero.
* Code adopted and modified from TinyMCE contextmenu plugin.

Anything missing?

dstillman added a commit that referenced this pull request Feb 5, 2014
- Send modifier keys through to loadURI() when clicking Open Link in notes
- Open link in parent window from external note window
- Don't show both menus on right-click

Follow-up from #450
dstillman added a commit that referenced this pull request Feb 5, 2014
TinyMCE Improvements: Open links from note window with popup and auto-create links
@dstillman dstillman closed this Feb 5, 2014
@dstillman
Copy link
Member

There already is a comment

Sorry, missed that.

Merged into 4.0. Note that I rebased your commit again to convert spaces to tabs (which are what we use) in linksmenu/editor_plugin.js, so you'll probably need to rewind to before these changes with git reset --hard <hash-from-before-the-changes> before pulling.

Thanks for working on this!

@dstillman
Copy link
Member

Now available for testing in 4.0.18-beta.r5.

@jlegewie
Copy link
Contributor Author

jlegewie commented Feb 5, 2014

Great! I will use and test it a little.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants