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

perf: reduce the number of left-click event listeners on files #148

Conversation

Turbinya
Copy link
Contributor

@Turbinya Turbinya commented Oct 6, 2021

Motivation

Improve performance and simplify code

Changes

Instead of hanging left-click event listeners on each file and every time the directory is changed, now only two click listeners (click and dbclick) are hung on their parents and only once. This allows significantly reduce the number of listeners created.

Additional Comments

I guess the same can be done for the context menu

@vercel
Copy link

vercel bot commented Oct 6, 2021

Someone is attempting to deploy a commit to a Personal Account owned by @kimlimjustin on Vercel.

@kimlimjustin first needs to authorize it.

@Turbinya Turbinya changed the title perf: reduce the number of right-click event listeners on files perf: reduce the number of left-click event listeners on files Oct 6, 2021
@Turbinya
Copy link
Contributor Author

Turbinya commented Oct 6, 2021

right and left confused lol

@vercel
Copy link

vercel bot commented Oct 6, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/kimlimjustin/xplorer/DuBNYD6LCyV17iwkVnjPYkvaQrCN
✅ Preview: https://xplorer-git-fork-turbinya-refactorright-cli-2c33d7-kimlimjustin.vercel.app

Copy link
Owner

@kimlimjustin kimlimjustin left a comment

Choose a reason for hiding this comment

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

Overall it's a good job!

However, IDK if you noticed it but whenever you reload, the settings button automatically add ups

Untitled_.Oct.6.2021.2_23.PM.mp4

Could you please fix this?

Thanks!

@kimlimjustin kimlimjustin added the changes requested Changes are needed for this pull request label Oct 6, 2021
@Turbinya
Copy link
Contributor Author

Turbinya commented Oct 6, 2021

Oops, ok. I will fix this problem a little later

@Turbinya
Copy link
Contributor Author

Turbinya commented Oct 8, 2021

@kimlimjustin Hi. What is the purpose of this feature. Why in reload() calls createSidebar()?

@kimlimjustin
Copy link
Owner

@kimlimjustin Hi. What is the purpose of this feature

WDYM?

What I meant is that your PR has a bug, which is the setting button (see the video left down area) is duplicating whenever I press the reload button, could you please fix it? (It's okay if you are not available, I could do it)

@Turbinya
Copy link
Contributor Author

Turbinya commented Oct 8, 2021

I understood what the problem is. I don't really understand why in reload() calls createSidebar(). Does the reload() exactly work as expected?

@kimlimjustin
Copy link
Owner

I understood what the problem is. I don't really understand why in reload() calls createSidebar(). Does the reload() exactly work as expected?

Eh, you remind me, calling the function in the reload function was because of the drive detection haven't been implemented at that time, and the drive detection works now, I think we can savely remove the function calling now :)

@Turbinya
Copy link
Contributor Author

Turbinya commented Oct 8, 2021

Ok. Now this problem is gone

Copy link
Owner

@kimlimjustin kimlimjustin left a comment

Choose a reason for hiding this comment

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

Yes, the setting problem gone, but here's another:

Comment on lines 74 to 86
const openFileHandler = (e:Event):void => {
let element = e.target as HTMLElement;
while (!element.dataset.path) {
element = element.parentNode as HTMLElement
}
const filePath = unescape(element.dataset.path)
const openFileHandler = (e: Event): void => {
const target = e.target as HTMLElement

// Ignore workspace clicks
if (target.id === 'workspace') return

const element = target.dataset.path ? target : target.parentNode as HTMLElement,
filePath = unescape(element.dataset.path);

Copy link
Owner

Choose a reason for hiding this comment

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

Changing this introduce another bug, when you click exactly on a drive's name, a error came out,

Untitled_.Oct.8.2021.7_15.PM.mp4

this is because the target I clicked, pendrive-title is under pendrive-info div that is under the div that contains the dataset.path, could you please fix this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, but today I changed my computer on which was Windows. Now i have a computer with Ubuntu. I didn't have such problems in Ubuntu. The computer with Windows will not be available to me for some time. So I cannot reliably verify that I have fixed it. So you can do it yourself or wait when I can.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And I faced two problems on Ubuntu which I will now describe

Copy link
Owner

Choose a reason for hiding this comment

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

Ah okay, I'll fix it tonight (if I got time) or tomorrow

@kimlimjustin kimlimjustin added approved and removed changes requested Changes are needed for this pull request labels Oct 9, 2021
@kimlimjustin kimlimjustin merged commit 3878a4a into kimlimjustin:master Oct 9, 2021
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.

2 participants