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

Fix navigation issues in file explorer #6552

Conversation

134130
Copy link
Contributor

@134130 134130 commented Apr 24, 2023

Description

  • Closes download-folder-button duplicated #6414
  • Reproducing route
    • Enter a file tree of repository.
    • Enter some file of the tree.
    • Pop to back page (I don't know the correct word of this)
    • Enter some file of the tree.

Test URLs

https://github.com/refined-github/refined-github/tree/main/source/features

Screenshot

Before

The button is duplicating and get pushed out to left
image

After

Remove previous button and stay the button right position.
Also, create button when accessing not only repo-tree but also single-file
image

@134130 134130 changed the title Fix cannot detect page redirection correctly Fix download-folder-button duplicating Apr 24, 2023
@fregante fregante added the bug label Apr 24, 2023
Copy link
Member

@fregante fregante left a comment

Choose a reason for hiding this comment

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

Unfortunately it's not that easy. The issue is not strictly related to download-folder-button so, if you want to look into it, you'd have to make sure that the logic in the feature manager is correct.

Another feature affected is:

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

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:

'[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

@fregante
Copy link
Member

I'm closing this PR because nothing can be used here unfortunately. If you'd like, maybe you can verify whether we can safely replace turbo:render with soft-nav:render everywhere` in our extension

@fregante fregante closed this Apr 25, 2023
@fregante fregante reopened this Apr 25, 2023
@fregante
Copy link
Member

I'm reopening this in case you want to keep experimenting or proposing new AJAX solutions

@fregante fregante changed the title Fix download-folder-button duplicating Fix navigation issues in file explorer Apr 25, 2023
@fregante fregante added the ajax Temporary label to collect Ajax issues label Apr 25, 2023
@fregante fregante marked this pull request as draft April 25, 2023 11:17
@fregante
Copy link
Member

Closing for now. If you have any suggestions let me know

@fregante fregante closed this May 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ajax Temporary label to collect Ajax issues bug
Development

Successfully merging this pull request may close these issues.

download-folder-button duplicated
2 participants