-
Notifications
You must be signed in to change notification settings - Fork 18
fix(Global/Vector-2022): 🐛 Site notice scrolling #786
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(Global/Vector-2022): 🐛 Site notice scrolling #786
Conversation
Change selectors to fix site notice scrolling under Vector-2022.
审阅者指南(在小型 PR 上折叠显示)审阅者指南调整 Vector-2022 的站点公告滚动选择器,使其针对新的容器结构,而不是旧的 body/content 布局,从而确保自动滚动依然有效,并且对自定义 moe 站点公告路径没有行为变化。 使用更新选择器后的 Vector-2022 站点公告滚动时序图sequenceDiagram
participant Browser
participant Script as Vector2022Script
participant DOM as Document
Browser->>Script: Load Vector-2022.js
Script->>DOM: startScroll() query
Note right of Script: Uses selector
Note right of Script: .vector-sitenotice-container > #siteNotice .scrollDiv:not(.scrolling)
DOM-->>Script: NodeList of scrollDiv elements
Script->>DOM: Add class scrolling
Script->>DOM: Normalize children (remove empty nodes)
Script->>Browser: setInterval(autoScroll)
loop Every interval
Script->>DOM: Check document.hidden
alt Page visible
Script->>DOM: autoScroll() query
Note right of Script: Uses selector
Note right of Script: .vector-sitenotice-container > #siteNotice .scrollDiv.scrolling
DOM-->>Script: NodeList of scrolling scrollDiv elements
Script->>DOM: Adjust scrollTop / position
else Page hidden
Script-->Script: Skip scrolling update
end
end
文件级变更
小贴士和命令与 Sourcery 交互
自定义你的体验访问你的 控制面板 来:
获取帮助Original review guide in EnglishReviewer's guide (collapsed on small PRs)Reviewer's GuideAdjusts the site notice scrolling selectors for Vector-2022 so they target the new container structure instead of the old body/content layout, ensuring auto-scrolling still works, with no behavioral changes to the custom moe sitenotice path. Sequence diagram for Vector-2022 site notice scrolling with updated selectorssequenceDiagram
participant Browser
participant Script as Vector2022Script
participant DOM as Document
Browser->>Script: Load Vector-2022.js
Script->>DOM: startScroll() query
Note right of Script: Uses selector
Note right of Script: .vector-sitenotice-container > #siteNotice .scrollDiv:not(.scrolling)
DOM-->>Script: NodeList of scrollDiv elements
Script->>DOM: Add class scrolling
Script->>DOM: Normalize children (remove empty nodes)
Script->>Browser: setInterval(autoScroll)
loop Every interval
Script->>DOM: Check document.hidden
alt Page visible
Script->>DOM: autoScroll() query
Note right of Script: Uses selector
Note right of Script: .vector-sitenotice-container > #siteNotice .scrollDiv.scrolling
DOM-->>Script: NodeList of scrolling scrollDiv elements
Script->>DOM: Adjust scrollTop / position
else Page hidden
Script-->Script: Skip scrolling update
end
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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.
Hey - 我发现了 1 个问题,并提供了一些总体层面的反馈:
- 更新后的 jQuery 选择器同时出现在
startScroll和autoScroll中;建议提取出公共的选择器字符串或辅助函数,这样如果未来需要修改就不会出现两处实现不一致的情况。 - 依赖
.vector-sitenotice-container > #siteNotice这个选择器与当前 DOM 结构高度耦合;如果可能,建议考虑使用更具弹性的选择器(例如基于一个不太可能变动的类名)以降低将来因布局更新导致的破坏风险。
给 AI Agent 的提示
Please address the comments from this code review:
## Overall Comments
- 更新后的 jQuery 选择器同时出现在 `startScroll` 和 `autoScroll` 中;建议提取出公共的选择器字符串或辅助函数,这样如果未来需要修改就不会出现两处实现不一致的情况。
- 依赖 `.vector-sitenotice-container > #siteNotice` 这个选择器与当前 DOM 结构高度耦合;如果可能,建议考虑使用更具弹性的选择器(例如基于一个不太可能变动的类名)以降低将来因布局更新导致的破坏风险。
## Individual Comments
### Comment 1
<location> `src/global/zh/Vector-2022.js:32` </location>
<code_context>
setInterval(() => {
if (!document.hidden) {
- $("body > #content > #siteNotice .scrollDiv.scrolling, #moe-sitenotice-container > .moe-sitenotice .scrollDiv.scrolling").each((_, ele) => {
+ $(".vector-sitenotice-container > #siteNotice .scrollDiv.scrolling, #moe-sitenotice-container > .moe-sitenotice .scrollDiv.scrolling").each((_, ele) => {
const self = $(ele);
if (self.css("font-weight") === "700") {
</code_context>
<issue_to_address>
**suggestion:** 考虑对 `startScroll` 和 `autoScroll` 之间共享的 sitenotice 选择器进行去重。
现在 sitenotice 的选择器在 `startScroll` 和 `autoScroll` 中被重复使用,只有 `.scrollDiv` 的状态不同。可以将共享的前缀提取为一个常量(例如 `const SITE_NOTICE_SELECTOR = '.vector-sitenotice-container > #siteNotice, #moe-sitenotice-container > .moe-sitenotice';`),以保证两个调用点保持同步,并在布局变更时减少维护开销。
建议实现:
```javascript
const SITE_NOTICE_SELECTOR = ".vector-sitenotice-container > #siteNotice, #moe-sitenotice-container > .moe-sitenotice";
const startScroll = () => {
```
```javascript
$(SITE_NOTICE_SELECTOR + " .scrollDiv:not(.scrolling)").addClass("scrolling").each((_, ele) => {
```
```javascript
$(SITE_NOTICE_SELECTOR + " .scrollDiv.scrolling").each((_, ele) => {
```
</issue_to_address>帮我变得更有用!请在每条评论上点 👍 或 👎,我会根据你的反馈改进后续的代码评审。
Original comment in English
Hey - I've found 1 issue, and left some high level feedback:
- The updated jQuery selector is duplicated in both
startScrollandautoScroll; consider extracting the common selector string or a helper function to avoid divergence if it needs to change again. - Relying on
.vector-sitenotice-container > #siteNoticeis fairly specific to the current DOM; if possible, consider a slightly more resilient selector (e.g., anchoring on a class that is less likely to change) to reduce future breakage from layout updates.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The updated jQuery selector is duplicated in both `startScroll` and `autoScroll`; consider extracting the common selector string or a helper function to avoid divergence if it needs to change again.
- Relying on `.vector-sitenotice-container > #siteNotice` is fairly specific to the current DOM; if possible, consider a slightly more resilient selector (e.g., anchoring on a class that is less likely to change) to reduce future breakage from layout updates.
## Individual Comments
### Comment 1
<location> `src/global/zh/Vector-2022.js:32` </location>
<code_context>
setInterval(() => {
if (!document.hidden) {
- $("body > #content > #siteNotice .scrollDiv.scrolling, #moe-sitenotice-container > .moe-sitenotice .scrollDiv.scrolling").each((_, ele) => {
+ $(".vector-sitenotice-container > #siteNotice .scrollDiv.scrolling, #moe-sitenotice-container > .moe-sitenotice .scrollDiv.scrolling").each((_, ele) => {
const self = $(ele);
if (self.css("font-weight") === "700") {
</code_context>
<issue_to_address>
**suggestion:** Consider de-duplicating the sitenotice selector shared between `startScroll` and `autoScroll`.
The sitenotice selector is now repeated in both `startScroll` and `autoScroll` with only the `.scrollDiv` state differing. Extract the shared prefix into a constant (e.g. `const SITE_NOTICE_SELECTOR = '.vector-sitenotice-container > #siteNotice, #moe-sitenotice-container > .moe-sitenotice';`) to keep both call sites in sync and reduce maintenance overhead when the layout changes.
Suggested implementation:
```javascript
const SITE_NOTICE_SELECTOR = ".vector-sitenotice-container > #siteNotice, #moe-sitenotice-container > .moe-sitenotice";
const startScroll = () => {
```
```javascript
$(SITE_NOTICE_SELECTOR + " .scrollDiv:not(.scrolling)").addClass("scrolling").each((_, ele) => {
```
```javascript
$(SITE_NOTICE_SELECTOR + " .scrollDiv.scrolling").each((_, ele) => {
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Change selectors to fix site notice scrolling under Vector-2022.
Summary by Sourcery
错误修复:
Original summary in English
Summary by Sourcery
Bug Fixes: