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

Remove jQuery .attr from the common admin functions #30115

Merged
merged 6 commits into from
Mar 27, 2024
Merged

Remove jQuery .attr from the common admin functions #30115

merged 6 commits into from
Mar 27, 2024

Conversation

yardenshoham
Copy link
Member

  • Switched from jQuery attr to plain javascript getAttribute and setAttribute
  • Tested most of the functions and they work as before

@yardenshoham
Remove jQuery .attr from the common admin functions
f8018f6 
- Switched from jQuery `attr` to plain javascript `getAttribute` and `setAttribute`
- Tested most of the functions and they work as before

Signed-off-by: Yarden Shoham <git@yardenshoham.com>
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Mar 26, 2024
@pull-request-size pull-request-size bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Mar 26, 2024
@yardenshoham yardenshoham added the type/refactoring Existing code has been cleaned up. There should be no new functionality. label Mar 26, 2024
@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Mar 26, 2024
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Mar 27, 2024
@lunny lunny added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Mar 27, 2024
@silverwind silverwind enabled auto-merge (squash) March 27, 2024 10:40
@silverwind
Copy link
Member

@lunny just enable auto-merge next time please. A lot of useless CI runs otherwise.

@silverwind silverwind merged commit a190f68 into go-gitea:main Mar 27, 2024
26 checks passed
@GiteaBot GiteaBot added this to the 1.23.0 milestone Mar 27, 2024
@GiteaBot GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Mar 27, 2024
@yardenshoham yardenshoham deleted the admin-common-jquery-attr branch March 27, 2024 10:56
silverwind added a commit to HEREYUA/gitea that referenced this pull request Mar 27, 2024
@silverwind
Merge branch 'main' into fix/modify_folded_review
c9f3ac5 
* main: (106 commits)
  Relax generic package filename restrictions (go-gitea#30135)
  Remove jQuery class from the common admin functions (go-gitea#30137)
  Remove jQuery class from the reaction selector (go-gitea#30138)
  Forbid jQuery `.attr` (go-gitea#30116)
  Refactor render (go-gitea#30136)
  Fix: Organization Interface Display Issue (go-gitea#30133)
  Remove jQuery `.attr` from the Fomantic dropdowns (go-gitea#30114)
  Remove jQuery `.attr` from the common admin functions (go-gitea#30115)
  Remove jQuery from the create/rename branch modals (except Fomantic) (go-gitea#30109)
  Remove fomantic label module (go-gitea#30081)
  Fix bug for markdown rendering of blockquote (go-gitea#30130)
  Fix: The interface is broken when modifying  code comments under mobile devices  (go-gitea#30125)
  When the title in the issue has a value, set the text cursor at the end of the text. (go-gitea#30090)
  Load attachments for code comments (go-gitea#30124)
  Upgrade fabric to 6.0.0-beta20 (go-gitea#30121)
  Fix click handler in job-step-summary (go-gitea#30122)
  Put an edit file button on pull request files to allow a quick operation (go-gitea#29697)
  Remove jQuery `.attr` from the Fomantic modal cancel buttons (go-gitea#30113)
  Remove jQuery `.attr` from the code comments (go-gitea#30112)
  Remove jQuery calls that have no effect on `showElem` and `hideElem` (go-gitea#30110)
  ...
silverwind added a commit to silverwind/gitea that referenced this pull request Mar 27, 2024
@silverwind
Merge remote-tracking branch 'origin/main' into drag
1e3f79a 
* origin/main: (108 commits)
  Fix table alignment classes (go-gitea#30144)
  Add default board to new projects, remove uncategorized pseudo-board (go-gitea#29874)
  Fix loading spinner on ContextPopup (go-gitea#30145)
  Fix download buttons on branches page (go-gitea#30147)
  Relax generic package filename restrictions (go-gitea#30135)
  Remove jQuery class from the common admin functions (go-gitea#30137)
  Remove jQuery class from the reaction selector (go-gitea#30138)
  Forbid jQuery `.attr` (go-gitea#30116)
  Refactor render (go-gitea#30136)
  Fix: Organization Interface Display Issue (go-gitea#30133)
  Remove jQuery `.attr` from the Fomantic dropdowns (go-gitea#30114)
  Remove jQuery `.attr` from the common admin functions (go-gitea#30115)
  Remove jQuery from the create/rename branch modals (except Fomantic) (go-gitea#30109)
  Remove fomantic label module (go-gitea#30081)
  Fix bug for markdown rendering of blockquote (go-gitea#30130)
  Fix: The interface is broken when modifying  code comments under mobile devices  (go-gitea#30125)
  When the title in the issue has a value, set the text cursor at the end of the text. (go-gitea#30090)
  Load attachments for code comments (go-gitea#30124)
  Upgrade fabric to 6.0.0-beta20 (go-gitea#30121)
  Fix click handler in job-step-summary (go-gitea#30122)
  ...
zjjhot added a commit to zjjhot/gitea that referenced this pull request Mar 28, 2024
@zjjhot
Merge remote-tracking branch 'giteaofficial/main'
fe778b8 
* giteaofficial/main: (63 commits)
  Apply to become a maintainer (go-gitea#30151)
  Refactor markdown render (go-gitea#30139)
  Drag-and-drop improvements for projects and issue pins (go-gitea#29875)
  Fix table alignment classes (go-gitea#30144)
  Add default board to new projects, remove uncategorized pseudo-board (go-gitea#29874)
  Fix loading spinner on ContextPopup (go-gitea#30145)
  Fix download buttons on branches page (go-gitea#30147)
  Relax generic package filename restrictions (go-gitea#30135)
  Remove jQuery class from the common admin functions (go-gitea#30137)
  Remove jQuery class from the reaction selector (go-gitea#30138)
  Forbid jQuery `.attr` (go-gitea#30116)
  Refactor render (go-gitea#30136)
  Fix: Organization Interface Display Issue (go-gitea#30133)
  Remove jQuery `.attr` from the Fomantic dropdowns (go-gitea#30114)
  Remove jQuery `.attr` from the common admin functions (go-gitea#30115)
  Remove jQuery from the create/rename branch modals (except Fomantic) (go-gitea#30109)
  Remove fomantic label module (go-gitea#30081)
  Fix bug for markdown rendering of blockquote (go-gitea#30130)
  Fix: The interface is broken when modifying  code comments under mobile devices  (go-gitea#30125)
  When the title in the issue has a value, set the text cursor at the end of the text. (go-gitea#30090)
  ...

# Conflicts:
#	templates/user/dashboard/issues.tmpl
@techknowlogick techknowlogick modified the milestones: 1.23.0, 1.22.0 Mar 28, 2024
@yp05327 yp05327 mentioned this pull request Apr 12, 2024
Closed
@silverwind silverwind mentioned this pull request Apr 12, 2024
Merged
lunny pushed a commit that referenced this pull request Apr 13, 2024
@silverwind
Fix admin notice view-detail (#30450)
8fd8978 
Fix #30434, regression from
#30115.

I also removed the date insertion into the modal which was also broken
since that date was switched to `absolute-date` because I see no real
purpose to putting that date into the modal.

Result:

<img width="1038" alt="image"
src="https://github.com/go-gitea/gitea/assets/115237/aa2eb8b4-73dc-4d98-9b80-3f276f89d9e5">
GiteaBot pushed a commit to GiteaBot/gitea that referenced this pull request Apr 13, 2024
@silverwind @GiteaBot
Fix admin notice view-detail (go-gitea#30450)
5e68a90 
Fix go-gitea#30434, regression from
go-gitea#30115.

I also removed the date insertion into the modal which was also broken
since that date was switched to `absolute-date` because I see no real
purpose to putting that date into the modal.

Result:

<img width="1038" alt="image"
src="https://github.com/go-gitea/gitea/assets/115237/aa2eb8b4-73dc-4d98-9b80-3f276f89d9e5">
@GiteaBot GiteaBot mentioned this pull request Apr 13, 2024
Merged
silverwind added a commit that referenced this pull request Apr 13, 2024
@GiteaBot @silverwind
Fix admin notice view-detail (#30450) (#30458)
fd2184e 
Backport #30450 by @silverwind

Fix #30434, regression from
#30115.

I also removed the date insertion into the modal which was also broken
since that date was switched to `absolute-date` because I see no real
purpose to putting that date into the modal.

Result:

<img width="1038" alt="image"
src="https://github.com/go-gitea/gitea/assets/115237/aa2eb8b4-73dc-4d98-9b80-3f276f89d9e5">

Co-authored-by: silverwind <me@silverwind.io>
DennisRasey pushed a commit to DennisRasey/forgejo that referenced this pull request Apr 16, 2024
@silverwind @algernon
Fix admin notice view-detail (#30450)
8673619 
Fix go-gitea/gitea#30434, regression from
go-gitea/gitea#30115.

I also removed the date insertion into the modal which was also broken
since that date was switched to `absolute-date` because I see no real
purpose to putting that date into the modal.

Result:

<img width="1038" alt="image"
src="https://github.com/go-gitea/gitea/assets/115237/aa2eb8b4-73dc-4d98-9b80-3f276f89d9e5">

(cherry picked from commit 8fd8978b4934865c2b041216e84e923ad574a4c7)
DennisRasey pushed a commit to DennisRasey/forgejo that referenced this pull request Apr 16, 2024
@GiteaBot @earl-warren
Fix admin notice view-detail (#30450) (#30458)
70b0c30 
Backport #30450 by @silverwind

Fix go-gitea/gitea#30434, regression from
go-gitea/gitea#30115.

I also removed the date insertion into the modal which was also broken
since that date was switched to `absolute-date` because I see no real
purpose to putting that date into the modal.

Result:

<img width="1038" alt="image"
src="https://github.com/go-gitea/gitea/assets/115237/aa2eb8b4-73dc-4d98-9b80-3f276f89d9e5">

Co-authored-by: silverwind <me@silverwind.io>
(cherry picked from commit fd2184e2343842e158f5b12e3d2e5b88effea2fd)
@wxiaoguang wxiaoguang mentioned this pull request Jun 4, 2024
Closed
wxiaoguang
wxiaoguang reviewed Jun 4, 2024
View reviewed changes
web_src/js/features/admin/common.js
showElem('.open_id_connect_auto_discovery_url');
break;
default:
if ($(`#${provider}_customURLSettings`).data('required')) {
$('#oauth2_use_custom_url').attr('checked', 'checked');
if (document.getElementById(`#${provider}_customURLSettings`)?.getAttribute('data-required')) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This change is wrong (#31244)

I would suggest to avoid using getElementById but only use queryXxx

Copy link
Member

Choose a reason for hiding this comment

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

Good catch. I think getElementById may have been used for performance, but as I now see it the difference is neglible: https://old.reddit.com/r/learnjavascript/comments/i0f5o8/performance_of_getelementbyid_vs_queryselector/

So I agree to refactor all getElementById to querySelector.

Copy link
Contributor

@wxiaoguang wxiaoguang Jun 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

And one more thing, we can't just do simple search&replace to refactor the legacy code. Actually there is not only one regression.

There are 2 more regressions here.

  • One is document.getElementById(`oauth2_${custom}`).value = document.getElementById(`${provider}_${custom}`).value; because the element may not exist.
  • Another one is if (el.getAttribute()) is also wrong (seel below)

Every line of refactored & changed code should be fully tested.

<a data-x="false">aaa</a>

const v1 = $('a')[0].getAttribute('data-x');
const v2 = $('a').data('x');
console.log(v1 ? 'yes' : 'no');
console.log(v2 ? 'yes' : 'no');

Copy link
Contributor

Choose a reason for hiding this comment

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

One more thought: if a page is quite complex and difficult to maintain (for example .... this "auth edit" page), it's better to refactor and rewrite it, instead of replacing legacy function calls. A fully rewritten page could be clearer and more correct than replacing the functions in the legacy code.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, we need to be aware of .data's "Every attempt is made to convert the attribute's string value to a JavaScript value" behaviour. It already came up.

Copy link
Member

@silverwind silverwind Jun 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

PS: I'm pretty sure document.querySelector('a').getAttribute wouldn't fly in Typescript because Typescript knows the querySelector can return null so would raise a type error, at least in strict mode.

Copy link
Member

Choose a reason for hiding this comment

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

I would suggest to avoid using getElementById but only use queryXxx

We could enable https://github.com/sindresorhus/eslint-plugin-unicorn/blob/main/docs/rules/prefer-query-selector.md which will be around 180 occurences currently, but I'm not sure if it's really worth it.

@wxiaoguang wxiaoguang mentioned this pull request Jun 4, 2024
Merged
@wxiaoguang
Copy link
Contributor

-> Fix admin oauth2 custom URL settings #31246

@GiteaBot GiteaBot mentioned this pull request Jun 4, 2024
Merged
@silverwind silverwind mentioned this pull request Jun 7, 2024
Merged
silverwind added a commit that referenced this pull request Jun 10, 2024
@silverwind @wxiaoguang @GiteaBot
Use querySelector over alternative DOM methods (#31280)
507fbf4 
As per
#30115 (comment),
prefer `querySelector` by enabling
[`unicorn/prefer-query-selector`](https://github.com/sindresorhus/eslint-plugin-unicorn/blob/main/docs/rules/prefer-query-selector.md)
and autofixing all except 10 issues.

According to
[this](https://old.reddit.com/r/learnjavascript/comments/i0f5o8/performance_of_getelementbyid_vs_queryselector/),
querySelector may be faster as well, so it's a win-win.

---------

Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>
Co-authored-by: Giteabot <teabot@gitea.io>
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Jun 25, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@wxiaoguang wxiaoguang wxiaoguang approved these changes

@silverwind silverwind silverwind approved these changes

Assignees
No one assigned
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. modifies/js size/L Denotes a PR that changes 100-499 lines, ignoring generated files. type/refactoring Existing code has been cleaned up. There should be no new functionality.
Projects
None yet
Milestone
1.22.0
Development

Successfully merging this pull request may close these issues.
6 participants
@yardenshoham @silverwind @wxiaoguang @lunny @techknowlogick @GiteaBot