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

Disabled select on row names & enhance simple click experience #1438

Merged
merged 5 commits into from
Aug 17, 2021

Conversation

RyRy79261
Copy link
Contributor

closes #1415

@RyRy79261 RyRy79261 added the Status: Review Needed 👀 Added to PRs when they need more review label Aug 11, 2021
@RyRy79261 RyRy79261 self-assigned this Aug 11, 2021
@render
Copy link

render bot commented Aug 11, 2021

@render
Copy link

render bot commented Aug 11, 2021

@render
Copy link

render bot commented Aug 11, 2021

@github-actions github-actions bot added the Type: Bug Fix Added to PRs if they are addressing a bug label Aug 11, 2021
Copy link
Collaborator

@Tbaut Tbaut left a comment

Choose a reason for hiding this comment

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

The problem with the selection is fixed perfectly, however with 150ms, this makes the double click way too quick, I barely manage to have a double click now, and the single click is still too slow.

I thought we could change our approach completely and use something like https://stackoverflow.com/a/66515641

We don't really care if the single click event is fired because it's only a selection, so if there's a double click, then it'll select and navigate/show preview.

What do you think?

@RyRy79261
Copy link
Contributor Author

Ahh ok, 150 was just the number that fit the people I tried it out on, nice find on that example there, I'll test it out n refactor if feasible

@Tbaut Tbaut added Status: Changes Requested ❌ Added to PRs that require further changes from the contributor. and removed Status: Review Needed 👀 Added to PRs when they need more review labels Aug 12, 2021
@RyRy79261
Copy link
Contributor Author

@Tbaut So the one issue I think we might run into is using the switch case, that it would trigger single click & double click actions

One option if this becomes an issue is to trigger the execution, alternatively being able to cancel the single click action if double is triggered

image

@RyRy79261 RyRy79261 requested a review from Tbaut August 12, 2021 13:37
@Tbaut
Copy link
Collaborator

Tbaut commented Aug 12, 2021

switch case, that it would trigger single click & double click actions

Yup, this is the behavior I meant to address with my

We don't really care if the single click event is fired because it's only a selection, so if there's a double click, then it'll select and navigate/show preview.

So I don't think it's actually a problem, but we should check how it behaves in real life to be certain it's really not an issue..

@RyRy79261
Copy link
Contributor Author

Ahh ok rad, yeah just thinking of use beyond our row selection but granted use cases where theres a single and a double tied to the same item are usually select or execute behaviours

@Tbaut Tbaut changed the title Disabled select on row names & set click to 150ms Disabled select on row names & enhance simple click experience Aug 12, 2021
Copy link
Collaborator

@Tbaut Tbaut left a comment

Choose a reason for hiding this comment

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

I think it's a great UX improvement for the simple click, and the double click just works fine from my tests 🎉

@Tbaut Tbaut added Status: Review Needed 👀 Added to PRs when they need more review and removed Status: Changes Requested ❌ Added to PRs that require further changes from the contributor. labels Aug 12, 2021
RyRy79261 and others added 2 commits August 12, 2021 17:22
Co-authored-by: Thibaut Sardan <33178835+Tbaut@users.noreply.github.com>
…SystemItem.tsx

Co-authored-by: Thibaut Sardan <33178835+Tbaut@users.noreply.github.com>
@RyRy79261
Copy link
Contributor Author

Rad! Thanks for the nifty approach, didn't realise events have context of other events

@asnaith
Copy link
Contributor

asnaith commented Aug 12, 2021

This is a nice improvement Ryan, the single click feels way better now 😃

@Tbaut
Copy link
Collaborator

Tbaut commented Aug 16, 2021

Can you please check the tests @RyRy79261 one seems to fail for valid reasons 🤷‍♂️

| 1) File management
| desktop
| can delete and recover a folder:
|
| AssertionError: Timed out retrying after 20000ms: Too many elements found. Found '2', expected '1'.
| + expected - actual
|
| -2
| +1

if (clickCount === 1) {
actionSingleClick && actionSingleClick(event)
const onClick = useCallback((e?: React.MouseEvent) => {
if (e) {
Copy link
Contributor

Choose a reason for hiding this comment

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

OMG! this is a gem! 💎
The events had the info all along!
How come the internet's full of these hook implementations!!!!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Tbaut Found this magic trick 🔥

I can't believe I only found this out now 😂

Copy link
Collaborator

Choose a reason for hiding this comment

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

Lol it's new for all of us, I didn't know either! Just I saw that Dropbox def. didn't have any timeout because their experience was great, and found that thing on wonderful SO :D

@asnaith
Copy link
Contributor

asnaith commented Aug 16, 2021

Can you please check the tests @RyRy79261 one seems to fail for valid reasons 🤷‍♂️

| 1) File management
| desktop
| can delete and recover a folder:
|
| AssertionError: Timed out retrying after 20000ms: Too many elements found. Found '2', expected '1'.
| + expected - actual
|
| -2
| +1

@Tbaut @RyRy79261 I took a quick look at this. It's a genuine failure but looks unrelated to Ryan's change. Files that shouldn't be there are present so when asserting that only the recovered folder exists it will fail because it sees other files.

The additional files that can be seen on the screenshot were not added during the test and the test begins with cy.web3Login({ clearCSFBucket: true, clearTrashBucket: true }) which should clear any pre-existing data. I can only assume the API call, to delete pre-existing files prior to the test beginning, did not work.

File management -- desktop -- can addremove multiple files and upload (failed)

@Tbaut Tbaut enabled auto-merge (squash) August 17, 2021 08:03
@Tbaut
Copy link
Collaborator

Tbaut commented Aug 17, 2021

Thanks @asnaith, then let's merge and create an issue for that.

edit: done #1448

@Tbaut Tbaut merged commit 194e302 into dev Aug 17, 2021
@Tbaut Tbaut deleted the fix/clicks-1415 branch August 17, 2021 08:06
@FSM1 FSM1 mentioned this pull request Sep 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Review Needed 👀 Added to PRs when they need more review Type: Bug Fix Added to PRs if they are addressing a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enhance navigation UX
4 participants