feat: separate show/hide animation durations and optional silent re-open - #807
Conversation
Adds three backwards compatible options to `animation`: - `showDuration` and `hideDuration` override `duration` for a single direction. Both default to null, so a config that only sets `duration` keeps using it for the show as well as the hide animation. - `animateOnReopen` (default true, i.e. current behaviour) controls whether the show animation is replayed when the menu that is being opened is already on screen, for instance when right clicking another element that shares the same menu. With it disabled the menu simply moves to the new position. Re-opening is detected on the menu element itself rather than on "some menu is open", so a different menu opening while another one closes is still animated. When the modal layer is used, opening another trigger waits for the hide animation to finish before showing the menu again, which is exactly what makes the animation replay. That wait is skipped when the same menu is re-opened with `animateOnReopen: false`, so the still visible menu is just repositioned.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ebf3977e2f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| // `selector` is either a selector string or, for menus registered on an | ||
| // element/jQuery object, the element(s) themselves - closest() takes both | ||
| return $(target).closest(root.selector).length > 0; |
There was a problem hiding this comment.
When two custom contexts register different menus with the same selector (for example, each container registers .trigger), this selector-only check classifies a click in the other context as reopening root's menu. The layer handler consequently opens the second menu immediately instead of waiting for the first menu's hide animation and layer cleanup, leaving both menus/effects active concurrently and potentially creating a second modal layer. Include the registration context or actual menu identity when deciding that the target reopens the same menu.
Useful? React with 👍 / 👎.
The selector-only check treated a trigger matching the same selector in another registration's context as a re-open of this menu, which would open that other menu before the current one finished hiding. Require the trigger to sit inside this registration's context as well.
|
Thanks, addressed the Codex review. Verify the target belongs to this menu before skipping the wait (P2) - valid, fixed in a376fb1. Not changed, for the record: the check stays selector plus context based rather than resolving the target's actual menu object. The plugin has no lookup from an arbitrary element to its registration ( |
|
👍 |
1 participant
Closes #739
Both requests from the issue, implemented backwards compatibly.
1. Don't replay the animation when the menu is already visible
New option
animation.animateOnReopen, defaulttrueso nothing changes for existing configs. With it set tofalse, opening a menu that is already on screen (right clicking another element that shares the same menu) moves it to the new position instead of playing the hide and show animation again.Two things were needed for that:
op.show()samples whetheropt.$menuitself is visible before anything is hidden, because hiding only starts an animation. This deliberately looks at the menu element, not at "is any menu open", so a different menu opening while another one closes is still animated. When the menu is being re-opened and re-animating is disabled, any hide animation is jumped to its end (its completion callbacks still run) and the menu is put back into its shown state synchronously, so it is never painted as hidden.useModal: true,handle.layerClick()waits forcontextmenu:hiddenbefore opening the menu on the new trigger. That wait is what makes the menu collapse and expand again, so it is skipped for this case and the menu is opened right after the hide is triggered. Menus built on invocation (build) still wait, since they get a brand new menu element every time and are torn down in the hide callback.2. Separate show and hide durations
New options
animation.showDurationandanimation.hideDuration, bothnullby default and falling back toanimation.duration, which keeps working exactly as before as a single value for both directions.0is honoured as an override rather than treated as unset.Tests
test/unit/issue-739-animation.test.js: default durations unchanged (asserted against the realslideDown/slideUpcalls),durationstill applying to both directions, per direction overrides, fallback behaviour, re-open still animating by default, and no replay withanimateOnReopen: false(including the menu never collapsing). Green on jQuery 1, 2, 3 and 4.test/specs/animation-reopen.js: real right click on a second trigger through the modal layer, sampling the menu while it is re-opened and asserting it never fades out or disappears. Green on jQuery 1.12.4, 2.2.4, 3.7.1 and 4.0.0. It fails onmaster.Docs
animationindocumentation/docs.mdnow documents all keys in a table, plus a new demo pagedocumentation/demo/animation.md(two triggers sharing one menu, fade in/out with different durations andanimateOnReopen: false). The documented default duration was also corrected from 500 to 50, which is what the source has always used.