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

Ensure double click event is not ignored in the browser tree. #8361

Merged
merged 1 commit into from
Jan 20, 2025

Conversation

yogeshmahajan-1903
Copy link
Contributor

No description provided.

Copy link
Contributor

@khushboovashi khushboovashi 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, below code would work rather then introducing the new code.
@yogeshmahajan-1903, check the code below, that should work.

private readonly handleClick = (ev: React.MouseEvent) => {
const { item, itemType, onClick } = this.props;
let isSingleClick;
if (isSingleClick = event.detail == 1){
setTimeout(() => {
if(isSingleClick){
if (itemType === ItemType.File || itemType === ItemType.Directory) {
console.warn("Handle Single Click");
onClick(ev, item as FileEntry, itemType);
}
}
}, 200);
}
else if (event.detail == 2) {
console.warn("Handle Double Click");
this.handleDoubleClick();
}
};

@yogeshmahajan-1903 yogeshmahajan-1903 force-pushed the master branch 3 times, most recently from bd753ed to 2201c68 Compare January 15, 2025 11:41
@yogeshmahajan-1903
Copy link
Contributor Author

I think, below code would work rather then introducing the new code. @yogeshmahajan-1903, check the code below, that should work.

private readonly handleClick = (ev: React.MouseEvent) => { const { item, itemType, onClick } = this.props; let isSingleClick; if (isSingleClick = event.detail == 1){ setTimeout(() => { if(isSingleClick){ if (itemType === ItemType.File || itemType === ItemType.Directory) { console.warn("Handle Single Click"); onClick(ev, item as FileEntry, itemType); } } }, 200); } else if (event.detail == 2) { console.warn("Handle Double Click"); this.handleDoubleClick(); } };

Having separate hook will be helpful if we need it in some other places too. I have sampled the code.

@khushboovashi
Copy link
Contributor

khushboovashi commented Jan 16, 2025

I think, below code would work rather then introducing the new code. @yogeshmahajan-1903, check the code below, that should work.
private readonly handleClick = (ev: React.MouseEvent) => { const { item, itemType, onClick } = this.props; let isSingleClick; if (isSingleClick = event.detail == 1){ setTimeout(() => { if(isSingleClick){ if (itemType === ItemType.File || itemType === ItemType.Directory) { console.warn("Handle Single Click"); onClick(ev, item as FileEntry, itemType); } } }, 200); } else if (event.detail == 2) { console.warn("Handle Double Click"); this.handleDoubleClick(); } };

Having separate hook will be helpful if we need it in some other places too. I have sampled the code.

I don't think we need a separate hook here, as it could be managed without a React state. The event itself gives the details of the click event, so what is the need to save it in a React state?

@adityatoshniwal
Copy link
Contributor

Having separate hook will be helpful if we need it in some other places too. I have sampled the code.

I don't think we need a separate hook here, as it could be managed without a React state.

It is important to cleanup setTimeout in React as it can lead to many adverse unknown effects. Having a hook is good for properly initiating and cleaning up things.

@khushboovashi
Copy link
Contributor

Having separate hook will be helpful if we need it in some other places too. I have sampled the code.

I don't think we need a separate hook here, as it could be managed without a React state.

It is important to cleanup setTimeout in React as it can lead to many adverse unknown effects. Having a hook is good for properly initiating and cleaning up things.

As I mentioned earlier, if the event itself gives the event details, there is no need to save it in the React state.

@adityatoshniwal
Copy link
Contributor

adityatoshniwal commented Jan 16, 2025

Having separate hook will be helpful if we need it in some other places too. I have sampled the code.

I don't think we need a separate hook here, as it could be managed without a React state.

It is important to cleanup setTimeout in React as it can lead to many adverse unknown effects. Having a hook is good for properly initiating and cleaning up things.

As I mentioned earlier, if the event itself gives the event details, there is no need to save it in the React state.

I'm talking about hook and setTimeout cleanup. Not talking about states.

@khushboovashi
Copy link
Contributor

Having separate hook will be helpful if we need it in some other places too. I have sampled the code.

I don't think we need a separate hook here, as it could be managed without a React state.

It is important to cleanup setTimeout in React as it can lead to many adverse unknown effects. Having a hook is good for properly initiating and cleaning up things.

As I mentioned earlier, if the event itself gives the event details, there is no need to save it in the React state.

I'm talking about hook and setTimeout cleanup. Not talking about states.

I am against Hook in this particular case.

@adityatoshniwal
Copy link
Contributor

Having separate hook will be helpful if we need it in some other places too. I have sampled the code.

I don't think we need a separate hook here, as it could be managed without a React state.

It is important to cleanup setTimeout in React as it can lead to many adverse unknown effects. Having a hook is good for properly initiating and cleaning up things.

As I mentioned earlier, if the event itself gives the event details, there is no need to save it in the React state.

I'm talking about hook and setTimeout cleanup. Not talking about states.

I am against Hook in this particular case.

As I mentioned earlier, hooks are good for properly initiating and cleaning up things at a single place. And they're re-usable. This is one case where we can introduce it and later use the same hook wherever needed. Duplication of logic/code should be avoided.

@khushboovashi khushboovashi merged commit 98f6b1f into pgadmin-org:master Jan 20, 2025
28 of 32 checks passed
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.

3 participants