-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Remove jQuery class from the notification count #30172
Remove jQuery class from the notification count #30172
Conversation
- Switched from jQuery class functions to plain JavaScript `classList` - Tested the notification count and it works as before Signed-off-by: Yarden Shoham <git@yardenshoham.com>
$notificationCount.text(`${data.new}`); | ||
for (const el of document.getElementsByClassName('notification_count')) { | ||
el.textContent = `${data.new}`; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess it's better to introduce a util function:
queryElem('.notification_count', (el) => el.textContent = `${data.new}`);
I have seen too many loops for the refactoring.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe export
Line 3 in 849eee8
function elementsCall(el, func, ...args) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, it is a good candidate, meanwhile I guess it needs some rewriting and renaming.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feel free to push here for nits
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would name it forElems
, e.g. do something for each matched element.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could also just use built-in methods, a bit more verbose but does the same:
Array.from(document.querySelectorAll('.foo')).forEach((el) => el.checked = true);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks there is NodeList.forEach
, so it can be shortened:
document.querySelectorAll('.foo').forEach((el) => el.checked = true);
@wxiaoguang @yardenshoham is that acceptable to you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could also just use built-in methods
Lint converts it into a for-of loop.
Looks there is NodeList.forEach, so it can be shortened
Honestly I think using a loop is OK, I don't see a big need to avoid it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm yes the rule thinks it's Array.forEach
because it has no type information and this is unfixable in the rule unless it gains type information from things like typescript's parser which we don't use yet.
So yes, I prefer revert to for-of
now, even if it's more verbose.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>
This reverts commit 99d4ea3.
* giteaofficial/main: Include encoding in signature payload (go-gitea#30174) Add `stylelint-value-no-unknown-custom-properties` and convert stylelint config to js (go-gitea#30117) Remove jQuery class from the commit button (go-gitea#30178) Remove jQuery class from the diff view (go-gitea#30176) Remove jQuery class from the notification count (go-gitea#30172) Remove jQuery class from the code range selection (go-gitea#30173) Fix:the rounded corners of the folded file are not displayed correctly (go-gitea#29953) Add setting to disable user features when user login type is not plain (go-gitea#29615) # Conflicts: # models/user/user.go
classList