-
-
Notifications
You must be signed in to change notification settings - Fork 8.9k
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
[JENKINS-72745] "Manage Jenkins" nested context menu can stay open when parent menu closes #10251
base: master
Are you sure you want to change the base?
Conversation
@@ -19,15 +19,26 @@ function generateDropdown(element, callback, immediate) { | |||
tippy( | |||
element, | |||
Object.assign({}, Templates.dropdown(), { | |||
hideOnClick: element.dataset["hideOnClick"] !== "false", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Used in
e.dataset["hideOnClick"] = "false"; |
I was unable to identify a difference in behavior as a result of this PR (e.g., in the "Copy From" textfield on the new item dialog), so perhaps I'm just missing how this is supposed to work? If it's not needed anymore or compensated for somehow by this new code, the data-
attribute should not longer be set.
Pinging @zbynek as the original author who might be able to help with this question.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we no longer read the data-
attribute, we can indeed remove the assignment 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@zbynek Thanks for confirmation.
Can you think of a widget that would be affected by the attribute, where this change may change behavior? The autocomplete popup of the "Copy from" text field doesn't seem to change behavior when I edit the data-
attribute in the DOM. I feel like I'm missing something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My PR disables Tippy's default hideOnClick
only for autocomplete, this PR disables it for all other dropdowns. So PR won't change behavior of autocomplete. It'ts the dropdowns that are not autocomplete that changed behavior in this PR:
For context menus clicking the chevron (downward triangle) the second time no longer closes the context menu. If that's still a desired behavior, the data-
attribute is still needed (and should be read in the click
handler to decide how to handle clicks in reference
element).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@zbynek Thanks for clarifying. That seems like a problem.
open.mov
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@daniel-beck I have updated the code to fix this problem. Thank you.
jenkins.mp4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ridemountainpig Thanks. Unfortunately now the fields where this was previously left open close when they shouldn't:
close.mov
That's what the hideOnClick
was used for. Could this behavior be retained, or does the fix for this require that we change one of the previous behaviors (stay open on autocomplete / close in other cases)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@daniel-beck I have updated it. Could you please check it again? Thank you.
jenkins.mp4
Appears to work great, thanks! 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this logic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like there is some formatting issues with prettier. From the logs we have:
[ERROR] Failed to execute goal com.github.eirslett:frontend-maven-plugin:1.15.1:corepack (prettier) on project jenkins-parent: Failed to run task: 'corepack yarn exec prettier --check .' failed. org.apache.commons.exec.ExecuteException: Process exited with an error: 1 (Exit value: 1) -> [Help 1]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. One potential performance issue is that each instance adds a new event listener to document
that never gets removed, but probably with the expected number of tooltips this won't cause visible slowdown.
const visibleDropdowns = Array.from(document.querySelectorAll('[data-tippy-root]')) | ||
.filter(dropdown => window.getComputedStyle(dropdown).visibility === 'visible'); | ||
|
||
const isClickInAnyDropdown = visibleDropdowns.some(dropdown => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why check for click on visible dropdowns and not all of them? I think something like
isClickInAnyDropdown = !!event.target().closest('[data-tippy-root]')
should be enough (?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have updated the code, and it works great. Thank you.
Does this interact poorly with widget refreshes every few seconds? |
The instances are created when you hover over the chevron, so if you keep the mouse cusor over the specific area and let the page reload a N times, you'll get N additional event handlers. |
See JENKINS-72745.
Testing done
CleanShot.2025-02-10.at.09.50.14.mp4
Proposed changelog entries
Proposed upgrade guidelines
N/A
Submitter checklist
Desired reviewers
@mention
Before the changes are marked as
ready-for-merge
:Maintainer checklist