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.

Sign up for GitHub

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

Jump to bottom

Fix navigation issues in file explorer #6552

Closed
wants to merge 3 commits into from
Closed

Fix navigation issues in file explorer #6552

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9564c65
Fix cannot detect page redirection correctly
134130 Apr 24, 2023
5746c4f
for refined button position
134130 Apr 24, 2023
81cb01c
lint
134130 Apr 24, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
12 changes: 7 additions & 5 deletions source/feature-manager.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -274,11 +274,13 @@ const add = async (url: string, ...loaders: FeatureLoader[]): Promise<void> => {
void setupPageLoad(id, details);
}

document.addEventListener('turbo:render', () => {
if (!deduplicate || !select.exists(deduplicate)) {
void setupPageLoad(id, details);
}
});
for (const eventType of ['turbo:render', 'soft-nav:render']) {
Copy link
Member

Choose a reason for hiding this comment

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

Both of these events are called when clicking one of the repos tabs, for example when you click on Issues. This means you're running every feature twice on every page load.

Also, neither one is called when you:

  1. Open https://github.com/refined-github/refined-github/tree/main/.github/workflows
  2. Go up one folder
  3. Go back in browser history ❌
  4. Go forward in browser history ❌

Copy link
Member

@fregante fregante Apr 25, 2023 •
edited

Choose a reason for hiding this comment

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

Ironically this feature is being duplicated here because our selector observer keeps working regardless of page changes, so it targets the "More options" dropdown every time React (mistakenly) regenerates it.

I might push a fix specific to this feature, but the lack of a global file listener is still unresolved. I think that as a starter, we need to replace turbo:render with soft-nav:render. Then maybe we can address the lack of browser history support by adding popstate listeners, but those need to be deduplicated when soft-nav:render is also fired.

Copy link
Member

Choose a reason for hiding this comment

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

I might push a fix specific to this feature

I "fixed the duplication" by using [title="More options"]:not(:has(+ .rgh-download-folder-button)), but this means that the old button is left in place, pointing to the old folder. 🥲

Let's move the discussion to:

document.addEventListener(eventType, () => {
if (!deduplicate || !select.exists(deduplicate)) {
void setupPageLoad(id, details);
}
});
}
}
};

Expand Down
6 changes: 5 additions & 1 deletion source/features/download-folder-button.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ import features from '../feature-manager';
import observe from '../helpers/selector-observer';

function add(folderDropdown: HTMLElement): void {
folderDropdown.parentElement!.querySelector('a.rgh-download-folder-button')?.remove();

const downloadUrl = new URL('https://download-directory.github.io/');
downloadUrl.searchParams.set('url', location.href);

Expand All @@ -24,14 +26,16 @@ function add(folderDropdown: HTMLElement): void {

function init(signal: AbortSignal): void {
observe([
'[title="More options"]',
'[title="More options"]', // PageDetect.isRepoTree
'[title="More file actions"]', // PageDetect.isSingleFile
'[aria-label="Add file"] + details', // TODO: Drop in mid 2023. Old file view #6154
], add, {signal});
}

void features.add(import.meta.url, {
include: [
pageDetect.isRepoTree,
pageDetect.isSingleFile,
Copy link
Member

Choose a reason for hiding this comment

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

This is a hack, a feature should not be enabled on random pages just to cleanup after itself

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but add singleFile is needed.
When we access blob direct link, Download button will not be rendered.

Copy link
Member

Choose a reason for hiding this comment

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

This feature must appear exclusively on directories because it downloads directories and the include array tells me exactly that.

It's only "needed" here because the feature manager is broken, but it's still a hack that we should not use

],
exclude: [
pageDetect.isRepoRoot, // Already has an native download ZIP button
Expand Down