Skip to content

[WIP] fix(sticky-header): Make the sticky header and the title header fit together better#1216

Open
Zes-MinKey-Young wants to merge 1 commit intoStarCitizenTools:mainfrom
Zes-MinKey-Young:main
Open

[WIP] fix(sticky-header): Make the sticky header and the title header fit together better#1216
Zes-MinKey-Young wants to merge 1 commit intoStarCitizenTools:mainfrom
Zes-MinKey-Young:main

Conversation

@Zes-MinKey-Young
Copy link
Contributor

@Zes-MinKey-Young Zes-MinKey-Young commented Dec 3, 2025

我又来了。

讲个笑话,我跑linter给我报了28351个错误,全是“行尾序列应为Unix风格(LF)”的错误😂。

我把标题栏进入和收回的缓动都改成了ease-out。(请看注释)尽管有人可能会认为这个不对称,我觉得还好,毕竟以前的版本其实也没有用ease-in。而且这样我们就可以用一个LESS混入类解决问题。

但是我遇到了点困难。我把sticky header的定位改为了top: 0,位置变换完全由transform处理。(您可以试试用top,它不如用transform严丝合缝。)这样一来虽然说贴合更好了,但是定位不太对了,如果快速翻向文件页顶部,这个吸顶的文件页目录会明显地跳一下。

一个解决办法是把标题栏改成也必须用top的。我不知道这合不合适。

请帮我解决这个问题,然后合并修改,谢谢。

Machine translated:

Hello again!

A (not so) little linting story:​ The linter reported 28,351 errors all of which were about "Line ending sequences should be Unix-style (LF)". Let's just say I had a fun time scrolling through those 😂.

Animation Easing:​ I've changed the easing curves for both the title bar's expand and collapse animations to ease-out. (Please see the code comments). While some might consider this asymmetrical, I think it's acceptable because the previous version didn't use ease-in either. The main advantage is that we can now handle both states using a single LESS mixin.

Sticky Header Positioning Challenge:​ I encountered a slight issue while working on the sticky header. I changed its positioning to use top: 0, intending to handle all movement solely through transform for better performance/consistency (You can try using top for animations; it's not as seamless as transform). While this makes the positioning tighter, it introduced a new problem. e.g. When quickly scrolling to the top of a file page, the sticky ToC now has a noticeable jump.


A potential fix is to adjust the title bar's styling so it mustalso be positioned using top. I'm not entirely sure if this is the appropriate approach or if it might have unintended side effects. Could you please advise on the best way to resolve this jumping issue?

Please help me fix the issue before merging. Thanks.

Summary by CodeRabbit

  • Style
    • Refined sticky header positioning to use transform-based offsetting for smoother, more consistent animations.
    • Introduced a new mobile header transition timing setting to standardize easing on small screens.
    • Updated transition timing for table of contents and page actions for improved responsiveness.
    • Removed a redundant transition rule to simplify header state transitions.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link

coderabbitai bot commented Dec 3, 2025

Warning

Rate limit exceeded

@alistair3149 has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 7 minutes and 31 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📥 Commits

Reviewing files that changed from the base of the PR and between 3073779 and fbcfb9c.

📒 Files selected for processing (3)
  • resources/mixins.less
  • resources/skins.citizen.styles/common/features.less
  • resources/variables.less

Walkthrough

Refactors mobile header animations: introduces a new LESS variable for mobile header easing, changes sticky header positioning from top-offset to transform-based offset, and standardizes transition-timing-function usage for header-related components.

Changes

Cohort / File(s) Change Summary
Sticky Header Positioning & Animation
resources/mixins.less
Replaced top: var(--header-offset-block-start) with top: 0 !important and added transform: translateY(var(--header-offset-block-start)). Switched animated property from top to transform, changed transition-timing-function to @mobile-header-transition-timing-function, kept transition-duration, and added explanatory comments.
Component Transition Timing
resources/skins.citizen.styles/common/features.less
Updated .citizen-toc and .page-actions transition-timing-function from var(--transition-timing-function-ease-out) to @mobile-header-transition-timing-function. Removed the header-down block that applied a different easing and its comment.
Shared Animation Variable
resources/variables.less
Added public LESS variable @mobile-header-transition-timing-function set to var(--transition-timing-function-ease-out) with comments documenting its intended use for mobile header elements.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

  • Check resources/mixins.less for correct transform/y-axis behavior and that translateY uses the intended CSS variable value.
  • Verify @mobile-header-transition-timing-function is declared before use and matches other easing semantics.
  • Confirm no regressions in header show/hide transitions across breakpoints (mobile vs desktop) where these selectors apply.

Possibly related PRs

Poem

🐰
I nudge the header, soft and spry,
from top to translateY I fly.
With one small easing, smooth and bright,
headers hop into the light. ✨

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly identifies the main change: improving sticky header and title header positioning and animation behavior through easing and positioning adjustments.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@Zes-MinKey-Young Zes-MinKey-Young changed the title fix(sticky-header): Make the sticky header and the title header fit together better [WIP] fix(sticky-header): Make the sticky header and the title header fit together better Dec 3, 2025
@Zes-MinKey-Young
Copy link
Contributor Author

Zes-MinKey-Young commented Dec 4, 2025

本地MediaWiki上做的测试:

top
https://github.com/user-attachments/assets/1a1aa50d-09bf-4e51-896d-1d3e5c265af5

transform:
https://github.com/user-attachments/assets/07ecab91-5c9f-4a5a-8483-13b1d5c54b05

(觉得您的头像很有意思就拿下来本地测试了,希望您别介意)

@alistair3149
Copy link
Member

Thanks for the PR and the detailed write-up!
I will do a deep dive next week since I have been busy recently.

Line ending sequences should be Unix-style (LF)".

.editorconfig should enforce LF when you open the repo in an IDE. What editor are you using?

end_of_line = lf

觉得您的头像很有意思就拿下来本地测试了,希望您别介意

I don't mind :)
I just thought that it is fitting as the GitHub avatar since GitHub's mascot is the Octocat.

// Instead, ease-out is actually better because fading out with ease-in is laggy.
// Also, using different timing functions for fade-in and fade-out creates the trouble
// that we cannot use a single mixin class `.mixin-citizen-sticky-header-element` for both cases.
@mobile-header-transition-timing-function: var( --transition-timing-function-ease-out );
Copy link
Member

Choose a reason for hiding this comment

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

We can just create a CSS variable (e.g. --transition-timing-function-nav ) directly. Using CSS var makes it accessible for interface admins on wiki, and we don't have to do the @import when we use it in other LESS styles.

For the name, mobile header is not quite right since the page sticky header also uses a transition in the desktop mode.

@Zes-MinKey-Young
Copy link
Contributor Author

Zes-MinKey-Young commented Dec 6, 2025

Thanks for the PR and the detailed write-up! I will do a deep dive next week since I have been busy recently.

Line ending sequences should be Unix-style (LF)".

.editorconfig should enforce LF when you open the repo in an IDE. What editor are you using?

end_of_line = lf

Visual Studio Code

@alistair3149
Copy link
Member

For VSC, you can change the current line style on the bottom right corner of the UI. Opening the whole repo as a folder should set the right settings too.

@alistair3149
Copy link
Member

The transition plays when the sticky header snaps back to the position, which looks a bit out of place:

Recording.2025-12-09.213317.mp4

@Zes-MinKey-Young
Copy link
Contributor Author

@alistair3149
在下重新捋了一遍。

目前3.11顶部标题栏用的是 transform 做变换,而文件页的 ToC 由于需要吸顶,用的是 top ,这两种定位方式在插值的时候有微小的差别,导致动画播放时不能严丝合缝地咬合在一起。

所以在我的提交中,我让 top 的值始终为 0,而用 transform 去控制 ToC 的动画。这样一来虽然说是严丝合缝了,但是对于吸顶ToC来说这样定位是不对的,在快速滑往顶端时非常明显,这个ToC会跳一下,而原有的 top 方案则不会。

如果标题顶栏也用 top 的话,应该可以很好的解决上面两种方案的缺陷,但是让已经是 transform 的顶栏去用 top 有点开历史倒车的意味(

@alistair3149
Copy link
Member

如果标题顶栏也用 top 的话,应该可以很好的解决上面两种方案的缺陷,但是让已经是 transform 的顶栏去用 top 有点开历史倒车的意味

That might be the reason why top was used in the first place, to make sure that it matches.
Personally, I think it is more important to avoid the jump in the sticky element, since it is more visually disruptive compared to the transition mismatch between the page header and the site header. What do you think?

@Zes-MinKey-Young
Copy link
Contributor Author

如果标题顶栏也用 top 的话,应该可以很好的解决上面两种方案的缺陷,但是让已经是 transform 的顶栏去用 top 有点开历史倒车的意味

That might be the reason why top was used in the first place, to make sure that it matches. Personally, I think it is more important to avoid the jump in the sticky element, since it is more visually disruptive compared to the transition mismatch between the page header and the site header. What do you think?

赞同,不过我们要不要把标题顶栏改成基于 top 动画的呢?

@alistair3149
Copy link
Member

如果标题顶栏也用 top 的话,应该可以很好的解决上面两种方案的缺陷,但是让已经是 transform 的顶栏去用 top 有点开历史倒车的意味

That might be the reason why top was used in the first place, to make sure that it matches. Personally, I think it is more important to avoid the jump in the sticky element, since it is more visually disruptive compared to the transition mismatch between the page header and the site header. What do you think?

赞同,不过我们要不要把标题顶栏改成基于 top 动画的呢?

Hmm that is a good question. I do want to avoid using top as much as possible because of the performance implication, even if it comes at a cost of minor visual discrepancy. Probably need some more opinion on this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants