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(Listbox): tap range select #1208

Merged
merged 8 commits into from Apr 3, 2023
Merged

feat(Listbox): tap range select #1208

merged 8 commits into from Apr 3, 2023

Conversation

DanielS-Qlik
Copy link
Contributor

@DanielS-Qlik DanielS-Qlik commented Mar 31, 2023

Motivation

Use two finger tap to select a range on touch devices.
fixes qlik-oss/sn-list-objects#163

Single-column:
single-column

Grid-column:
grid-column

Grid-row:
grid-row

Requirements checklist

  • Api specification
    • Ran yarn spec
      • No changes OR API changes has been formally approved
  • Unit/Component test coverage
  • Correct PR title for the changes (fix, chore, feat)

When build and tests have passed:

  • Add code reviewers, for example @qlik-oss/nebula-core

@DanielS-Qlik DanielS-Qlik marked this pull request as ready for review March 31, 2023 11:50
Copy link
Collaborator

@T-Wizard T-Wizard left a comment

Choose a reason for hiding this comment

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

Have you tested on a real touch device?

I think it will work if you don't move the fingers during the tap.
but I am unsure what happens if you do. (and if that is what we want)

@DanielS-Qlik
Copy link
Contributor Author

Have you tested on a real touch device?

I think it will work if you don't move the fingers during the tap. but I am unsure what happens if you do. (and if that is what we want)

Yes I tested it on a iPad. I noticed that the old listbox seems to prevent gestures (like zoom) inside the listbox, so I added that as well. ffaf757

I have also tried to update the touch positions with onTouchMove, to get the latest positions if you move during tap.
But I only get the start position for some reason.

@@ -16,6 +23,25 @@ export default function useSelectionsInteractions({ selectionState, selections,
touchRangeSmall: false,
});

useEffect(() => {
if (!loaderRef.current?._listRef?._outerRef) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

what is this for?

listRef.removeEventListener('gesturechange', preventGestureChange);
listRef.removeEventListener('gestureend', preventGestureEnd);
};
}, [loaderRef.current?._listRef?._outerRef]);
Copy link
Collaborator

Choose a reason for hiding this comment

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

this can maybe be a problem as
loaderRef.current?._listRef?._outerRef could probably without updating the effect
if the update do not also trigger a rerender of the listbox

@DanielS-Qlik DanielS-Qlik merged commit 957d96d into master Apr 3, 2023
3 checks passed
@DanielS-Qlik DanielS-Qlik deleted the ild/tap-range-select branch April 3, 2023 13:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Double tap does not work on touch
2 participants