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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Use AJAX navigation for default-branch-button #4631

Merged
merged 2 commits into from
Aug 4, 2021
Merged

Use AJAX navigation for default-branch-button #4631

merged 2 commits into from
Aug 4, 2021

Conversation

dertieran
Copy link
Contributor

@dertieran dertieran commented Aug 2, 2021 •
edited

Fixes #4623

I just took the attribute from the master element and it worked 馃槄
It is also used for other elements so it seems to be the right way to trigger an AJAX navigation.

Test URLs

https://github.com/vutran/twas/tree/v2.1.2

Screenshot

ajax.mp4

Browser(s) used

Chromium 92.0.4515.107

yakov116
yakov116 approved these changes Aug 2, 2021
View reviewed changes
Copy link
Member

@yakov116 yakov116 left a comment

Choose a reason for hiding this comment

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

馃帀

@dertieran dertieran marked this pull request as ready for review August 2, 2021 14:01
@fregante
Copy link
Member

fregante commented Aug 2, 2021

Supper-lative! Are there any other Refined elements that would benefit from this attribute? I think most buttons we add on the repo could have that (like latest-tag-button). It might be worth looking for them

@dertieran dertieran force-pushed the default-branch-button-use-pjax branch from 62479a0 to b691e5a Compare August 3, 2021 06:51
@dertieran
Copy link
Contributor Author

Okay so I found out you can also just add a (truthy) data-pjax attribute and it would just make an AJAX.
(I think it just used the #js-repo-pjax-container in that case, maybe because of the data-pjax-container attribute)

But I'm not sure if I find the time to scan the code to add it to all elements.
If somebody want to add them feel free to do so, or I can also add them if I know the features and what container to use.
Otherwise I will see if I find the time later to look into it :)

@fregante fregante merged commit c201bf2 into refined-github:main Aug 4, 2021
@fregante
Copy link
Member

fregante commented Aug 4, 2021

Thanks for the PRs Philipp!

kidonng
kidonng reviewed Aug 4, 2021
View reviewed changes
source/features/latest-tag-button.tsx
@@ -132,6 +136,7 @@ async function init(): Promise<false | void> {
<a
className="btn btn-sm btn-outline tooltipped tooltipped-ne"
href={buildRepoURL(`compare/${latestTag}...${defaultBranch}`)}
data-pjax="#repo-content-pjax-container"
Copy link
Member

@kidonng kidonng Aug 4, 2021 •
edited

Choose a reason for hiding this comment

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

This looks wrong:

image

Changing it to #js-repo-pjax-container works but file diffs remain there when you navigate to another page

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True I totally overlooked that the header is duplicated 馃槄

This was referenced Aug 4, 2021
Closed
Merged
@dertieran dertieran mentioned this pull request Aug 4, 2021
Closed
@dertieran dertieran deleted the default-branch-button-use-pjax branch August 4, 2021 14:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kidonng kidonng kidonng left review comments

@yakov116 yakov116 yakov116 approved these changes

Assignees
No one assigned
Labels
enhancement
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

default-branch-button does not use AJAX navigation
4 participants
@dertieran @fregante @yakov116 @kidonng