WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 9 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 (18)

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

Download all attachments as: .zip

Change History (55)

@welcher
3 years ago

#1 follow-up: @welcher
3 years ago

  • Keywords has-patch added

#2 in reply to: ↑ 1 @adamsilverstein
3 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
3 years ago

cache $(menuMarkup)

#3 @adamsilverstein
3 years ago

In 31218.2.diff:

  • cache the $(menuMarkup) selector

#4 @welcher
3 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
3 years ago

  • Keywords dev-feedback added

#6 @welcher
3 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 3 years ago by welcher (previous) (diff)

@welcher
3 years ago

Updated JSDOCS

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


3 years ago

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


3 years ago

#9 @azaozz
3 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
3 years ago

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

@welcher
3 years ago

#11 follow-up: @adamsilverstein
3 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
3 years ago

Add JSDocks to addMenuToTop

#12 in reply to: ↑ 11 @welcher
3 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
3 years ago

Changing the name of the event being triggered

#13 @welcher
3 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
3 years ago

Updated JSDOCS to reflect event name change

#14 @welcher
3 years 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
3 years 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.


2 years ago

#17 @chriscct7
2 years ago

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

#18 @welcher
2 years 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.


2 years ago

#20 @jorbin
2 years 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
2 years ago

  • Keywords early added

#22 @chriscct7
2 years ago

  • Keywords next-release added; early removed

#23 @meatloaf1024
19 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
19 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
19 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
18 months ago

31218.6.diff

  • Add some QUnit tests for the expected event triggers

#27 @adamsilverstein
18 months ago

31218.7.diff:

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

#28 @adamsilverstein
18 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
18 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
18 months ago

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

#31 @adamsilverstein
18 months ago

31218.9.diff:

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

#32 @adamsilverstein
18 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
18 months ago

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

#34 @adamsilverstein
17 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.

#35 @adamsilverstein
9 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening because we are seeing some intermittent test failures in Travis -

https://cl.ly/0w3Y3H0z3h2X/Job_1069.1_-_WordPresswordpress-develop_-_Travis_CI_2017-10-02_08-36-04.jpg

Expected 3 assertions, but 5 were run - looks like this is happening because the delayed test that verifies all hooks have fired is running too early.

#36 @adamsilverstein
9 months ago

In 31218.12.diff increase the timeout for the wpNavMenu Qunit test to 3 seconds

#37 @adamsilverstein
9 months ago

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

In 41675:

Menus: Increase timeout for QUnit wpNavMenu trigger tests.

Fix an (intermittent) issue where the final assertion verifying all hooks have been triggered ran too early, resulting in the test failing.

Fixes #31218.

Note: See TracTickets for help on using tickets.