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

Fixed an issue with not removing the resize handler when user clicks RMB #10416

Merged
merged 8 commits into from Jun 20, 2023

Conversation

wszymanski
Copy link
Contributor

@wszymanski wszymanski commented Jun 15, 2023

Context

This PR adds callback for right click on resize handler. It fixes problem described within https://github.com/handsontable/dev-handsontable/issues/1312.

The resize handler should be deactivated upon right mouse button (RMB) click by the user. Although it does not result in any technical problems, its presence primarily affects the overall user experience of the component. Currently, when the RMB is clicked and the menu is closed, the resizing handle remains visible and follows the movement of the mouse cursor.

See below a video showing the problem, mentioned in the issue.

move-rmb.mp4

How has this been tested?

Tested case described in the issue.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature or improvement (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Additional language file or change to the existing one (translations)

Related issue(s):

  1. https://github.com/handsontable/dev-handsontable/issues/1312

Affected project(s):

  • handsontable
  • @handsontable/angular
  • @handsontable/react
  • @handsontable/vue
  • @handsontable/vue3

Checklist:

  • I have reviewed the guidelines about Contributing to Handsontable and I confirm that my code follows the code style of this project.
  • My change requires a change to the documentation.

@wszymanski wszymanski self-assigned this Jun 15, 2023
@wszymanski wszymanski marked this pull request as ready for review June 15, 2023 14:34
@wszymanski wszymanski requested a review from budnix June 15, 2023 14:34
Copy link
Member

@budnix budnix left a comment

Choose a reason for hiding this comment

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

The fix seems to be working. Can you apply the same fix for the ManualRowResize plugin?

Comment on lines +574 to +575
this.hot.rootElement.removeChild(this.handle);
this.hot.rootElement.removeChild(this.guide);
Copy link
Member

@budnix budnix Jun 16, 2023

Choose a reason for hiding this comment

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

Suggested change
this.hot.rootElement.removeChild(this.handle);
this.hot.rootElement.removeChild(this.guide);

I think that it is ok that the handler does not disappear after triggering the context menu. The origin bug is still fixed, the handler does not follow the mouse after unclicking the context menu:
Kapture 2023-06-16 at 10 22 56

By removing these lines, we do not add new code to maintain, and the fix still seems to be working. So why not do it 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As you can see, there UI element left behind. Title of the issue says that: "The resize handler should be removed when user clicks RMB".

Copy link
Member

@budnix budnix Jun 16, 2023

Choose a reason for hiding this comment

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

For better code health, I'd remove the lines. UI, in this case, still works fine for me. What do you think @AMBudnik @krzysztofspilka?

Copy link
Member

Choose a reason for hiding this comment

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

To clarify. Current PR fixes the issue by hiding/removing the handler right after the RMB is clicked. I believe it's a bad UI/UX design that some elements just disappear after mouse-clicking.
Kapture 2023-06-16 at 12 34 01

My proposition is not to hide/remove the handler and make it just inactive for RMB.
Kapture 2023-06-16 at 12 36 29

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 fine with me 👍

Copy link
Member

Choose a reason for hiding this comment

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

@budnix @wszymanski @AMBudnik

Thanks for discussing this! I think it all comes down to two options that we have:

  1. Leave the resize indicator as it is when the context menu is opened, as @budnix suggested - but then, for the sake of consistency, allow the end-user to resize other columns manually. It's what Google Sheets does.
google-beh.mp4

or

  1. Hide the manual-resize indicator. The mouse cursor should return to default in this case.
excel-beh.mp4

Solution no. 2 blocks the UI outside (behind) the context menu, which is exactly what the OS-context menu does. It's also more consistent with how Handsontable interacts with other plugins. For example, in v12.4.0 a mouse cursor over the dropdown menu button doesn't give you the on-hover visual feedback. The cursor style remains default.

That being said, I'm fond of solution no. 2 (hide the manual-resize indicator).

See the screen recording below for reference.

Screen.Recording.2023-06-17.at.11.58.06.mov

@wszymanski
Copy link
Contributor Author

wszymanski commented Jun 16, 2023

The fix seems to be working. Can you apply the same fix for the ManualRowResize plugin?

So far changes are done without remark from: #10416 (comment). I'll do changes related to than when needed (after a decision).

@wszymanski wszymanski added the Blocked Issue that is being blocked by another one or an epic label Jun 16, 2023
@wszymanski wszymanski requested a review from budnix June 20, 2023 08:32
Copy link
Member

@budnix budnix left a comment

Choose a reason for hiding this comment

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

👌

@wszymanski wszymanski merged commit 31c3354 into develop Jun 20, 2023
21 checks passed
@wszymanski wszymanski deleted the feature/dev-issue-1312 branch June 20, 2023 08:54
@wszymanski wszymanski removed the Blocked Issue that is being blocked by another one or an epic label Jun 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants