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

Remove jQuery .attr from the commit graph #30006

Merged
merged 5 commits into from
Mar 22, 2024
Merged

Remove jQuery .attr from the commit graph #30006

Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d4af6c0
Remove jQuery `.attr` from the commit graph
yardenshoham Mar 22, 2024
5d11f86
different style
yardenshoham Mar 22, 2024
cca306b
Revert "different style"
yardenshoham Mar 22, 2024
258df41
Merge branch 'main' into repo-graph-jquery-attr
GiteaBot Mar 22, 2024
80c4555
Merge branch 'main' into repo-graph-jquery-attr
GiteaBot Mar 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions web_src/js/features/repo-graph.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,27 +18,27 @@ export function initRepoGraphGit() {
window.history.replaceState({}, '', window.location.pathname);
}
$('.pagination a').each((_, that) => {
const href = $(that).attr('href');
const href = that.href;
silverwind marked this conversation as resolved.
Show resolved Hide resolved
if (!href) return;
const url = new URL(href, window.location);
const params = url.searchParams;
params.set('mode', 'monochrome');
url.search = `?${params.toString()}`;
$(that).attr('href', url.href);
that.href = url.href;
Copy link
Member

Choose a reason for hiding this comment

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

Use setAttribute instead of href. There are slight differences in those two methods in how the URL encodes as I recently noticed.

Copy link
Member

Choose a reason for hiding this comment

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

Ah I see @wxiaoguang suggests the opposite.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you elaborate, what's the difference? https://developer.mozilla.org/en-US/docs/Web/API/HTMLAnchorElement/href

Copy link
Member

Choose a reason for hiding this comment

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

@wxiaoguang see #29931 (comment). There I came to the conclusion it's better to use setAttribute and getAttribute when working with href.

Copy link
Member

@silverwind silverwind Mar 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

That's the difference:

console.log(a.href);
// https://fiddle.jshell.net/_display/?editor_console=true#%E6%B0%94%E5%80%99%E5%AE%9C%E4%BA%BA

console.log(a.getAttribute("href"));
//  #气候宜人

href can not deal with unicode and resolves to absolute URL.

Copy link
Member

@silverwind silverwind Mar 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I do not think so. el.href is the normalized and correctly escaped full URL. It could just be used as-is.

The method to choose depends on use case whether absolute escaped or relative unicode URL is needed. We should just do what jQuery did before.

@yardenshoham could you debug it out what jQuery did please? I assume it was equivalent to getAttribute and setAttribute.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it essentially calls getAttribute.
https://github.com/jquery/jquery/blob/f79d5f1a337528940ab7029d4f8bbba72326f269/src/attributes/attr.js#L68
https://github.com/jquery/jquery/blob/f79d5f1a337528940ab7029d4f8bbba72326f269/src/selector.js#L875

Copy link
Member

Choose a reason for hiding this comment

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

Yeah so let's change it back so it's a more accurate refactor?

Copy link
Contributor

@wxiaoguang wxiaoguang Mar 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

In your example, a.setAttribute("href", "#气候宜人") you are setting a invalid URL to href. So when you read the el.href again, it gets correctly escaped.

It's not a invalid URL as URL constructor accepts it:

URL also generates the normalized and fully escaped URL: href: 'https://example.com/#%E6%B0%94%E5%80%99%E5%AE%9C%E4%BA%BA'. So I insist el.href is the right thing to be directly used.

image

Copy link
Member

Choose a reason for hiding this comment

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

Both will work I assume. That said, this code looks ripe for refactor, these two functions are almost completely the same.

});
});
$('#flow-color-colored').on('click', () => {
$('#flow-color-colored').addClass('active');
$('#flow-color-monochrome').removeClass('active');
$('#git-graph-container').addClass('colored').removeClass('monochrome');
$('.pagination a').each((_, that) => {
const href = $(that).attr('href');
const href = that.href;
if (!href) return;
const url = new URL(href, window.location);
const params = url.searchParams;
params.delete('mode');
url.search = `?${params.toString()}`;
$(that).attr('href', url.href);
that.href = url.href;
});
const params = new URLSearchParams(window.location.search);
params.delete('mode');
Expand Down