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

feat: add middle click dashboard tab deletion #1992

Merged

Conversation

AkshatJawne
Copy link
Contributor

Resolves #1990

  • Add functionality to close dashboard tab with middle mouse click by specifying onAuxClick event
  • Create NavBar component test cases that test changes

@AkshatJawne AkshatJawne self-assigned this May 7, 2024
Copy link
Contributor

github-actions bot commented May 7, 2024

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@@ -67,6 +67,12 @@ const NavTab = memo(
data-testid={`btn-nav-tab-${title}`}
role="tab"
tabIndex={0}
onAuxClick={event => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think onAuxClick works in safari.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will take a look into this

Copy link
Collaborator

Choose a reason for hiding this comment

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

In that case you probably will need onMouseUp or onMouseDown (check behavior of the golden-layout tabs) and check event.buttons===4 I think

Copy link
Contributor

Choose a reason for hiding this comment

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

It's 1 not 4, and you should be able to get it from just the click event I would think.

Copy link
Contributor

@bmingles bmingles May 7, 2024

Choose a reason for hiding this comment

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

Tab component seems to use:

this.element.on('click', this._onTabClick);
this.element.on('auxclick', this._onTabClick);

@dsmmcken any idea if jQuery is making auxclick work in Safari, or is there something else happening here? I don't see anything special otherwise.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you check middle click to close actually works on Safari with golden layout? If so, then jQuery is polyfilling the functionality. If not, then we just never tested it works in Safari

You would want to check both button and buttons on mouseUp because you could start dragging, then middle click on accident and possibly get into a very weird state. You just need to check 2 values, so I'm not sure how it's that complicated

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looking into it currently, will update

Copy link
Contributor

Choose a reason for hiding this comment

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

Middle click does work with Safari golden layout.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm following now. Seems like mouseUp event with a button === 1 && buttons === 0 check is the simplest approximation. Just means that if you click down elsewhere and drag onto the tab and then release it would close the tab, but that doesn't really seem like a concern.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Spoke to Matt, pushing changes shortly.

@AkshatJawne AkshatJawne changed the title feat:add middle click dashboard tab deletion feat: add middle click dashboard tab deletion May 7, 2024
@AkshatJawne
Copy link
Contributor Author

I have read the CLA Document and I hereby sign the CLA

@mattrunyon
Copy link
Collaborator

recheck

@AkshatJawne
Copy link
Contributor Author

recheck

@AkshatJawne AkshatJawne force-pushed the 1990-middle-click-dashboard-close branch from 6ff5f98 to 538632c Compare May 7, 2024 22:28
deephaven-internal added a commit to deephaven/cla that referenced this pull request May 7, 2024
Copy link
Contributor

@bmingles bmingles left a comment

Choose a reason for hiding this comment

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

Changes look good. I verified everything works as expected on Chrome + Safari.

Left minor suggestion for comment tweaks

packages/components/src/navigation/NavTab.tsx Outdated Show resolved Hide resolved
packages/components/src/navigation/NavTab.tsx Outdated Show resolved Hide resolved
Copy link
Contributor

@bmingles bmingles left a comment

Choose a reason for hiding this comment

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

Looks good

@AkshatJawne AkshatJawne merged commit c922f87 into deephaven:main May 9, 2024
9 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators May 9, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow middle clicking a dashboard top tab to close the dashboard
4 participants