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

Implement support for Toolbar and Context Menu plugins #438

Open
wants to merge 8 commits into
base: develop
Choose a base branch
from

Conversation

dhughes-xumak
Copy link
Contributor

@dhughes-xumak dhughes-xumak commented Jul 18, 2017

Related to #435. This adds options for easily defining additional buttons to the Toolbar and Context Menu. Though multi-select is currently broken, this should support additional custom Context Menu options for multiple selections.


This contribution is graciously provided by XumaK (http://xumak.com/)

XumaK helps reduce the uncertainty and risk that is normally involved for companies trying to transform themselves digitally.

We provide professional services teams with years of experience to deliver the project, using our end-to-end delivery platform, which ensures your project will be on time and on budget. We deliver world class customer solutions through innovative industry leading products.

@josdejong
Copy link
Owner

Thanks a lot, appreciated!

I will review your PR in-depth soon, it looks well taken care of at first sight.

@dhughes-xumak
Copy link
Contributor Author

Great, thanks @josdejong!

We appreciate your quick action and on-going maintenance of this project.

@dhughes-xumak
Copy link
Contributor Author

dhughes-xumak commented Jul 18, 2017

Some other considerations to improve this feature:

  • Add {string} contextMenuPlugins.submenuTitle property to the test data and documentation.
  • Add {function} contextMenuPlugins.enabled property so each plugin can decide if it should be included in a given context menu. This probably makes it possible to combine contextMenuPlugins and multiContextMenuPlugins into one list.
  • Convert the built-in context menu actions to use the new plugin architecture (depends on the first two points).
  • Add a property for defining hotkeys for toolbar and context menu plugins.

Admittedly, my javascript is really rusty and I had some trouble with the context menu click handlers. This is why they are being wrapped at 2a6a65e#diff-0e52f886d57f8a2c0f91f314276ac9a9R1295. I'm sure the handlers could be constructed without needing to wrap them.

  • Get rid of the callback wrapping for contextMenuPlugins/multiContextMenuPlugins. If the selected node(s) are passed to the ContextMenu constructor, then the wrapping would not be necessary.

Edit: added two commits to address two of those points.

  • Add {string} contextMenuPlugins.submenuTitle property to the test data and documentation.
  • Get rid of the callback wrapping for contextMenuPlugins/multiContextMenuPlugins. If the selected node(s) are passed to the ContextMenu constructor, then the wrapping would not be necessary.

Edit: added one more commit.

  • Add {function} contextMenuPlugins.enabled property so each plugin can decide if it should be included in a given context menu.
    This should make it possible to combine contextMenuPlugins and multiContextMenuPlugins into one configuration list, if desired.

…lick handlers.

This allows us to remove the ugly callback wrapping when processing the contextMenuPlugins.
Also updates test and documentation.
I noticed that the arrow only renders properly in some cases.  For this reason, the submenuTitle only works sometimes.
@dhughes-xumak dhughes-xumak force-pushed the feature/toolbar-and-contextmenu-plugins branch from cc82e94 to fbd082d Compare July 18, 2017 21:07
…e themselves.

Includes docs and tests.

In the future, it may be preferable for disabled actions to be constructed and appear in
the context menu, appearing as disabled (grayed out).
Copy link
Contributor Author

@dhughes-xumak dhughes-xumak left a comment

Choose a reason for hiding this comment

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

Note for unrelated clean up.

@@ -44,6 +44,20 @@ var util = require('./util');
* {boolean} sortObjectKeys If true, object keys are
* sorted before display.
* false by default.
* {Object[]} toolbarPlugins Array of custom toolbar
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This docblock is outdated and should be removed entirely. API Doc should be sufficient for documenting this.

This was referenced Jul 20, 2017
@josdejong
Copy link
Owner

josdejong commented Jul 21, 2017

I've just checked out your PR @dhughes-xumak, thanks a lot again! You have implemented a solid API, you really know what you're doing and the code looks good.

On a side note: I'm working on a complete new version of the JSONEditor, "v6", see #337. I had planned features that you're implementing right now for v6.1. This means that we'll have to re-implement them some day for v6 (which is ok, most work is figuring out the right API, not the code itself).

Here some points of feedback:

  1. For consistency, it may be better to rename the click callbacks in toolbarPlugins, contextMenuPlugins, and multiContextMenuPlugins all to onClick. Currently the first is onclick and the others are click following the documentation. So far the API is camelCase everywhere, so that's probably the most consistent naming to apply for onClick too.

  2. Currently, the onClick callbacks pass the current node/nodes. The the Node class is non-public and fully undocumented, not really suitable API for public use. That will lead to questions and issues due to misunderstanding or misusing the API. Instead, we could pass the current JSON path instead of the node, like other API's do (onEditable and the onChange of templates). Maybe also rename selectedElements to selectedPaths in the code, and to make it easier to reason about, always pass an array with 1 or multiple paths?

  3. Nice that you've added some validation in _processContextMenuPlugin!

  4. Currently you've implemented toolbarPlugins for the tree mode only. I think we also need it for other modes. We will need a way to configure in whether a toolbar button must be displayed in a specific mode or not. Could be by introducing an extra config option modes: [...] which defaults to all modes, or by passing the current mode with to the callback function that creates the button, so it can decide to return null when in a mode where the button is not applicable.

  5. Can you add an example on how to use toolbar plugins, context menu plugins, and multi context menu plugins in the examples section? (Basically move test_plugins.html to examples, rename it and cleanup a bit where needed).

  6. Currently all actions show green "+" icons (see screenshot). Instead, we should display no icon by default. And probably implement a way to specify custom icons yourself.
    image

  7. Currently, when the text of a menu doesn't fit, it's aligned on the next line which looks quite weird (see "Submenu+click" in the screenshot above). We should do some CSS fixes there.

  8. Multi-select nodes by dragging is currently broken in the editor, so I can't test whether the plugins work there. I haven't looked into the issue itself yet.

EDIT: we can discuss point 6 and 7 in #440

@dhughes-xumak
Copy link
Contributor Author

Currently all actions show green "+" icons (see screenshot). Instead, we should display no icon by default. And probably implement a way to specify custom icons yourself.

Commit bde68cf adds a jsoneditor-plugin class and the corresponding styles to remove the default '+' icon.

Currently, it sets the icon background to transparent for context menus and #d3d3d3 for toolbars. If a plugin icon is added to the icon sheet, that should be used as the default.

Updated test_plugins to test and demonstrate icon overrides for plugins.

@josdejong
Copy link
Owner

@dhughes-xumak That looks way better already :)

I've just fixed the issue with the broken multiselection (see 2110d25, will do a release today), and have done some adjustments in the css of the context menu so it wraps long text neatly.

This resolves points 6, 7, and 8.

So the left over action points are now:

  1. Rename click callbacks to onClick for all three menus.
  2. Pass JSON path instead of Node to onClick
  3. Implement support for toolbar plugins in all modes
  4. Add an example in the examples section
  5. Integrate undo/redo functionality into the plugins

@josdejong
Copy link
Owner

(the merge conflict that's there now is just two new blocks of CSS inserted at the same location)

@josdejong
Copy link
Owner

@dhughes-xumak this PR is still open, are you still interested in completing it?

@dhughes-xumak
Copy link
Contributor Author

No, sorry. It would be great to see this included in the project, but I will not be completing it.

@josdejong
Copy link
Owner

Ok clear! At least we have a nice start :)

If anyone is interested in finishing this feature please let me know.

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

Successfully merging this pull request may close these issues.

2 participants