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

Restore `toggle-everything-with-alt` feature #2087

Open
wants to merge 16 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@notlmn
Copy link
Contributor

commented May 28, 2019

Closes #1729.
Implements and closes #825.

The feature now allows collapsing/expanding:

notlmn added some commits May 28, 2019

Show resolved Hide resolved source/features/toggle-all-things-with-alt.tsx Outdated
Show resolved Hide resolved source/features/toggle-all-things-with-alt.tsx Outdated
Show resolved Hide resolved source/features/toggle-all-things-with-alt.tsx Outdated
Show resolved Hide resolved source/features/toggle-all-things-with-alt.tsx Outdated
Show resolved Hide resolved source/features/toggle-all-things-with-alt.tsx Outdated

notlmn and others added some commits May 28, 2019

notlmn added some commits May 29, 2019

@bfred-it

This comment has been minimized.

Copy link
Collaborator

commented May 30, 2019

  1. In Firefox, clicking any "Show comment" in #1694 freezes the browser for 4 seconds in my case, but not in Chrome. Can you look into it?

  2. Also review comments don't work in Chrome and Firefox, here https://github.com/sindresorhus/refined-github/pull/1896/files

  3. The suggested link for large diffs only has one large diff on the page: https://github.com/parcel-bundler/parcel/pull/2967/files

@bfred-it bfred-it force-pushed the sindresorhus:master branch from 5df5c5f to 5f1a966 Jun 1, 2019

@notlmn

This comment has been minimized.

Copy link
Contributor Author

commented Jun 2, 2019

3. The suggested link for large diffs only has one large diff on the page: parcel-bundler/parcel/pull/2967/files

There are multiple of them on that page. It is the same as loading diffs for deleted files: https://github.com/sindresorhus/refined-github/pull/1524/files.

image


  1. In Firefox, clicking any "Show comment" in #1694 freezes the browser for 4 seconds in my case, but not in Chrome. Can you look into it?

Happens to me on Chrome too. Not a bug, just main thread jank for clicking on 90 elements all at once. (Any solution?)


2. Also review comments don't work in Chrome and Firefox, here sindresorhus/refined-github/pull/1896/files

Missed that, added it in.

@bfred-it

This comment has been minimized.

Copy link
Collaborator

commented Jun 2, 2019

Happens to me on Chrome too. Not a bug, just main thread jank for clicking on 90 elements all at once. (Any solution?)

It's worth investigating if it's the click or the selection being slow

@notlmn

This comment has been minimized.

Copy link
Contributor Author

commented Jun 2, 2019

It's the clicks, events being triggered recursively. But the functions early return anyway.

image

@bfred-it

This comment has been minimized.

Copy link
Collaborator

commented Jun 4, 2019

Should we space them out / delay them a bit? We can't cause multi-second freezes.

In this case I'd probably click no more than 3 per second, also because it's gonna take longer than that to open + see them anyway, but only the "Load diff". You can probably implement this by using await delay(300) in the loop: https://www.npmjs.com/package/delay

Additionally, perhaps, it could be limited to 10 files at a time, but this code might be a bit ugly and maybe not worth it.

@bfred-it

This comment has been minimized.

Copy link
Collaborator

commented Jun 6, 2019

Also can you rename this to toggle-everything-with-alt?

@bfred-it bfred-it added the enhancement label Jun 7, 2019

notlmn added some commits Jun 7, 2019

@bfred-it bfred-it changed the title Restore `toggle-all-things-with-alt` feature Restore `toggle-everything-with-alt` feature Jun 22, 2019

notlmn added some commits Jun 23, 2019

@bfred-it

This comment has been minimized.

Copy link
Collaborator

commented Jul 10, 2019

Can you add the same mechanism here? https://github.com/sindresorhus/refined-github/pull/2073/files#diff-bfeb5de166cf585c43d9150166877a54R89

Except spaced every 3 and using requestAnimationFrame instead

@notlmn

This comment has been minimized.

Copy link
Contributor Author

commented Jul 10, 2019

Can you add the same mechanism here? sindresorhus/refined-github/pull/2073/files#diff-bfeb5de166cf585c43d9150166877a54R89

The handlers are delegated over to .repository-content, so this works even for ajaxed file diffs in a PR even without using that util.


Except spaced every 3 and using requestAnimationFrame instead

I don't seem to follow you on this.

If you are talking about #2087 (comment), then that's not possible, as the possible solution in #2073 mentioned would not work here. Spacing out callback to let the user scroll will cause the last scroll position to be invalidated and when the callback is eventually fired in rAF, the page scrolls to random positions for each callback.

Having a jank is pretty reasonable IMHO (instead of handling scroll position changes for random file load jumps).

if (item !== clickedItem) {
item.click();
}
}

This comment has been minimized.

Copy link
@bfred-it

bfred-it Jul 11, 2019

Collaborator

I don't think we should ever cause multi-second locks, it's just bad, especially because it will look like the page is unresponsive and the user might alt-click it again and... now the queue is longer.


It's a little more complicated because each batch would have to be surrounded by a get scroll and set scroll, but the anchorScroll we have in #2146 should help.

const chunked = chunkArray(similarItems.filter(item => item !== clickedItem), 3);
for (const chunk of chunked) {
	await anchorScroll(() => {
		for (const item of chunk) {
			item.click();
		}
	});
}

Or maybe an easier way to just process 3 at a time with a regular for loop

This comment has been minimized.

Copy link
@notlmn

notlmn Jul 11, 2019

Author Contributor

It's a little more complicated because each batch would have to be surrounded by a get scroll and set scroll

Yup, that's the problem. But its more like guarding each call to .click() and its respective layout change, while handling scroll position changes.


but the anchorScroll we have in #2146 should help.

That doesn't seem to help much too, the scroll position ends up being at some random positions after triggering the clicks.

I've tried handling duplicate events, any events emitted on repeated clicks, chunking callbacks, even plain promises, and all of them have the same symptom, the scroll position ends up being at random positions (especially for chunked triggers and allowing user scrolling without jank).

This comment has been minimized.

Copy link
@bfred-it

bfred-it Jul 14, 2019

Collaborator

the scroll position ends up being at some random positions after triggering the clicks

Indeed, our function only works with sync changes but in this case the fragments might be loaded asynchronously so our "reset position on rAF" was executed before the fragment loaded. I dropped it in c2c44b1. Do you think it's still useful for anything in this feature

Anyway it looks like the browser's scroll anchoring already takes care of it if the elements are completely outside the viewport.

@bfred-it

This comment has been minimized.

Copy link
Collaborator

commented Jul 11, 2019

There might have been some miscommunication. My comment only was referring to the await promise setTimeout mechanism (652bb9e#diff-bfeb5de166cf585c43d9150166877a54R90) but the link I had posted pointed to the wrong line (probably after a commit changed its line)

@bfred-it bfred-it self-assigned this Jul 14, 2019

bfred-it added some commits Jul 14, 2019

Drop unnecessary scroll anchor
It looks like the browser takes care of it already when items are outside the viewport
Avoid freezing the browser for too long in Firefox
`.click` can take up to 60ms in Firefox so it's best to space them out a bit... but only if necessary: in Chrome the operation takes 2ms
Avoid freezing the browser for too long in Firefox /2
It should keep track of the time "since the last frame," not of each operation separately
@bfred-it

This comment has been minimized.

Copy link
Collaborator

commented Jul 14, 2019

I implemented the mechanism I mentioned and but now it only awaits if the operation is taking a while. It might be worth implementing the same thing in #2073 as well instead of having the arbitrary 100 lines limit.

@bfred-it

This comment has been minimized.

Copy link
Collaborator

commented Jul 14, 2019

In the meanwhile I think GH changed the selectors because this doesn't work anymore:

Hidden comments (test: #1694

And this one is not in your test list at all:

review comments per-file ("Show comments" checkbox): sindresorhus/refined-github/pull/1896/files

Also feel free to restore c2c44b1 if necessary anywhere (but the re-set needs to happen before/inside the await and the new viewportOffset needs to be updated after it)

@bfred-it bfred-it removed their assignment Jul 14, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.