WordPress.org

Make WordPress Core

Opened 2 years ago

Closed 6 months ago

#31218 closed enhancement (fixed)

nav-menu.js menu item added event

Reported by: welcher Owned by: adamsilverstein
Milestone: 4.8 Priority: normal
Severity: trivial Version: 4.2
Component: Menus Keywords: has-patch next-release
Focuses: javascript Cc:

Description

I am writing a plugin that adds a checkbox to each item in the admin Menu that is being edited. The checkbox is used to either bulk delete items in the menu or to insert a new one after it. Here is the plugin on github - https://github.com/ryanwelcher/Bulk-Remove-Menu-Items

Currently, I am overriding the addMenuToBottom() method in nav-menu.js to achieve the goal. I would love to see an event be triggered after the item is added. This seems like a much more efficient and elegant - not to mention less hacky - way of approaching this.

I have attached a patch with an initial approach that passes the markup of the item as a parameter. Any input is welcome.

Attachments (17)

31218.diff (537 bytes) - added by welcher 2 years ago.
31218.2.diff (1.1 KB) - added by adamsilverstein 2 years ago.
cache $(menuMarkup)
31218_3.diff (1.1 KB) - added by welcher 2 years ago.
Updated JSDOCS
31218_4.diff (1.5 KB) - added by welcher 2 years ago.
31218_5.diff (1.5 KB) - added by adamsilverstein 2 years ago.
31218_6.diff (1.8 KB) - added by welcher 2 years ago.
Add JSDocks to addMenuToTop
31218_7.diff (1.8 KB) - added by welcher 2 years ago.
Changing the name of the event being triggered
31218_8.diff (1.8 KB) - added by welcher 2 years ago.
Updated JSDOCS to reflect event name change
31218.3.diff (1.8 KB) - added by adamsilverstein 8 months ago.
31218.4.diff (1.7 KB) - added by adamsilverstein 8 months ago.
31218.5.diff (2.2 KB) - added by adamsilverstein 8 months ago.
31218.6.diff (4.7 KB) - added by adamsilverstein 7 months ago.
31218.7.diff (5.0 KB) - added by adamsilverstein 7 months ago.
31218.8.diff (5.0 KB) - added by adamsilverstein 6 months ago.
31218.9.diff (5.0 KB) - added by adamsilverstein 6 months ago.
31218.10.diff (5.0 KB) - added by adamsilverstein 6 months ago.
31218.11.diff (5.0 KB) - added by adamsilverstein 6 months ago.

Download all attachments as: .zip

Change History (51)

@welcher
2 years ago

#1 follow-up: @welcher
2 years ago

  • Keywords has-patch added

#2 in reply to: ↑ 1 @adamsilverstein
2 years ago

Hey Ryan!

Thanks for the patch. Interesting idea for a plugin, the menu system certainly gets unwieldy when long (or always :)).

I think triggering an event on document is perfectly reasonable, although if you are going to reuse the $(menuMarkup) selector it should be cached. I also see this as another argument for standardizing Actions and Filters in JavaScript in core; until that bolder budges, triggering on document is the way to go.

@adamsilverstein
2 years ago

cache $(menuMarkup)

#3 @adamsilverstein
2 years ago

In 31218.2.diff:

  • cache the $(menuMarkup) selector

#4 @welcher
2 years ago

Thanks for the suggestions and patch edits @adamsilverstein.

I also wanted to add this changeset from the plugin I am working on that demonstrates the differences based on the availability of an event:

https://github.com/ryanwelcher/Bulk-Remove-Menu-Items/compare/core-event

#5 @welcher
2 years ago

  • Keywords dev-feedback added

#6 @welcher
2 years ago

Adding an .gif to demonstrate how my plugin responds to the event being fired. The checkboxes are added by the plugin and when checked, the new menu item is moved below the checked item after being added to the menu.

http://ryanwelcher.com/backstage/wp-content/uploads/demo-video.gif

Last edited 2 years ago by welcher (previous) (diff)

@welcher
2 years ago

Updated JSDOCS

This ticket was mentioned in Slack in #core by welcher. View the logs.


2 years ago

This ticket was mentioned in Slack in #core by welcher. View the logs.


2 years ago

#9 @azaozz
2 years ago

Agree with @adamsilverstein, don't see a problem adding another "global" event. The thing is when this code changes these events will most likely go, i.e. it's not possible to have backwards-compatibility similar to PHP's actions and filters.

#10 @welcher
2 years ago

Updating patch to add a similar event to the addMenuItemToTop() method as per @azaozz's comments in Slack.

@welcher
2 years ago

#11 follow-up: @adamsilverstein
2 years ago

31218_5.diff: slight code standards cleanup (whitespace!).

@welcher - Thanks for the JS docs, now seems sorely lacking from the addMenuItemToTop function, can you add that? :)

@welcher
2 years ago

Add JSDocks to addMenuToTop

#12 in reply to: ↑ 11 @welcher
2 years ago

Replying to adamsilverstein:

31218_5.diff: slight code standards cleanup (whitespace!).

@welcher - Thanks for the JS docs, now seems sorely lacking from the addMenuItemToTop function, can you add that? :)

Patch updated. I also remove an extraneous parameter in the JS docs that didn't exists in either method declaration and indicated how each of the method adds the new menu item.

@welcher
2 years ago

Changing the name of the event being triggered

#13 @welcher
2 years ago

I have changed the name of the event being triggered to match the menu-customizer plugin. If this is committed into core now then any plugins/themes that responding to the event can still do so once it the admin page is replaced by the customizer.

@welcher
2 years ago

Updated JSDOCS to reflect event name change

#14 @welcher
20 months ago

  • Milestone Awaiting Review deleted
  • Resolution set to wontfix
  • Status changed from new to closed

No movement and extremely out of date. Closing.

#15 @welcher
19 months ago

  • Milestone set to 4.5
  • Resolution wontfix deleted
  • Status changed from closed to reopened

Thought about it and I think this is still valid. Patch applies still.

This ticket was mentioned in Slack in #core by chriscct7. View the logs.


17 months ago

#17 @chriscct7
17 months ago

  • Owner set to chriscct7
  • Status changed from reopened to reviewing

#18 @welcher
17 months ago

@chriscct7 let me know if I can do anything further to move this along. Open to suggestions as well.

This ticket was mentioned in Slack in #core by jorbin. View the logs.


17 months ago

#20 @jorbin
17 months ago

  • Milestone changed from 4.5 to Future Release

No momentum pushing this over the line, so it's going to miss 4.5.

#21 @chriscct7
17 months ago

  • Keywords early added

#22 @chriscct7
17 months ago

  • Keywords next-release added; early removed

#23 @meatloaf1024
8 months ago

  • Severity changed from normal to trivial

Is there any movement on this? I am creating a little plugin that could really benefit from this functionality.

#24 @adamsilverstein
8 months ago

  • Keywords dev-feedback removed

@welcher in 31218.5.diff I updated the patch with another event right before items are removed from the menu, again passing a jQuery object handle to the menu DOM element itself. @jorbin pointed out this might be useful and I agree.

I took the brackets out of the data passed in your trigger calls as it was passing the element as a single element array.

Can you review and see if this still works for your use case?

#25 @welcher
7 months ago

@adamsilverstein tested the patch against my plugin and other than a minor tweak to get it working - I had to wrap the menuMarkup in $() - it works exactly as expected in my plugin.

Thanks!

#26 @adamsilverstein
7 months ago

31218.6.diff

  • Add some QUnit tests for the expected event triggers

#27 @adamsilverstein
7 months ago

31218.7.diff:

  • QUnit tests: fail if the events aren't triggered
Last edited 7 months ago by adamsilverstein (previous) (diff)

#28 @adamsilverstein
6 months ago

  • Milestone changed from Future Release to 4.8
  • Owner changed from chriscct7 to adamsilverstein

@welcher can you try the unit tests and verify they make sense and work correctly for you?

thanks.

#29 @welcher
6 months ago

@adamsilverstein I ran the tests locally and ran into errors. Specifically:

Uncaught TypeError: assert.done is not a function@ 122 ms
Source:
http://localhost:63342/src/tests/qunit/wp-admin/js/nav-menu.js:8

and then on subsequent loads:

Uncaught TypeError: Cannot read property 'equal' of undefined@ 182 ms
Source:
http://localhost:63342/src/tests/qunit/wp-admin/js/nav-menu.js:7

This is my first time running the QUnit sweet so it may be my setup. I am running them in the browser.

#30 @adamsilverstein
6 months ago

31218.8.diff corrects the issue you were seeing @welcher, can you try again please?

#31 @adamsilverstein
6 months ago

31218.9.diff:

  • Cleanup
  • Increase timeout to 1500 ms to ensure events have a chance to fire

#32 @adamsilverstein
6 months ago

In 31218.10.diff:

  • Switch the failure callback to assert that the expected number of tests had completed. No longer tries to clear the timeout.

With the patch, all tests pass:
https://cl.ly/0v183o04342Q/Image%202017-01-16%20at%2011.53.51%20AM.png

Without the patch, the expected events don't fire and the fail check fails:
https://cl.ly/0F2c251h0t1M/Image%202017-01-16%20at%2011.54.26%20AM.png

#33 @welcher
6 months ago

I just ran the tests locally and everything seems a-ok.

#34 @adamsilverstein
6 months ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

In 39928:

Menus: trigger an event when menu items are added or removed.

Fire a menu-item-added event after a menu item is added to the DOM. Fire a menu-removing-item event before a menu item is removed from the DOM. Enables hooking into and responding to menu changes.

Props welcher, adamsilverstein.
Fixes #31218.

Note: See TracTickets for help on using tickets.