-
Notifications
You must be signed in to change notification settings - Fork 688
Add sticky header to PR/Issue views #8285
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
Conversation
Co-authored-by: alexr00 <[email protected]>
Co-authored-by: alexr00 <[email protected]>
Co-authored-by: alexr00 <[email protected]>
Co-authored-by: alexr00 <[email protected]>
Co-authored-by: alexr00 <[email protected]>
alexr00
left a comment
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.
@copilot, if the scroll is just at the threshold if flickers back in forth constantly.
Co-authored-by: alexr00 <[email protected]>
Fixed in e9e5ee1. Added hysteresis with a 10px buffer zone - the header now activates sticky mode at 80px when scrolling down, but only deactivates when scrolling back up past 70px. This prevents flickering when hovering around the threshold. |
Co-authored-by: alexr00 <[email protected]>
daimandsukesh-ai
left a comment
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.
Sorry
alexr00
left a comment
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.
@copilot It still flickers like crazy when I scroll down.
Co-authored-by: alexr00 <[email protected]>
Fixed in cb46ded. Completely redesigned the approach - the flickering was caused by JavaScript changing the header's margins/padding, which shifted the page layout and scroll position, creating a feedback loop. New solution:
The header now smoothly transitions without any flickering when scrolling. |
alexr00
left a comment
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.
@copilot Nice, there's no flickering now. Please fix the following:
- The subtitle no longer shows.
- Most of the other header styling is no longer applied
- Don't show boders/shadows on the top and sides of the stuck section.
Co-authored-by: alexr00 <[email protected]>
Fixed in 515a52c. Changes made:
The header now displays correctly with all original styling, and the subtitle is visible until the header becomes stuck. |
alexr00
left a comment
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: alexr00 <[email protected]>
Fixed in 030cf1c. Completely replaced the IntersectionObserver approach with scroll-based position detection. New approach:
This is more reliable because it directly checks the title's actual position rather than trying to infer it from a sentinel element's intersection state. |
Co-authored-by: alexr00 <[email protected]>
alexr00
left a comment
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.
@copilot that works great! Also hide the edit title button when in sticky mode.
Co-authored-by: alexr00 <[email protected]>
alexr00
left a comment
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.
@copilot oh come on. Don't access an html element by it's title! Give it an ID, put the ID In /webviews/common/constants.ts.
Co-authored-by: alexr00 <[email protected]>
Fixed in 86aa087. Changes made:
The button is now properly identified by its ID rather than the title attribute. |
| </h2> | ||
| {canEdit ? | ||
| <button title="Rename" onClick={() => setEditMode(true)} className="icon-button"> | ||
| <button id={EDIT_TITLE_BUTTON_ID} title="Rename" onClick={() => setEditMode(true)} className="icon-button"> |
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.
Do these strings (such as Rename here) get localized?
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.
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.
No, there isn't a good story for localizing strings in webviews.
Implementation Plan for Sticky PR/Issue Headers
Summary
Successfully implemented sticky PR/Issue headers similar to GitHub.com:
position: stickywithout layout-changing JavaScriptrequestAnimationFramethrottling detects when header is stuckgetBoundingClientRect()to check actual element position (not sentinel-based)Technical Approach
The implementation uses:
position: stickyon the header at all timesrequestAnimationFramethrottling for optimal performancegetBoundingClientRect()instead of IntersectionObservertitle.classList.remove('stuck')on mount to ensure clean initial stateSTICKY_THRESHOLD = 1constant to account for sub-pixel renderingrect.top <= STICKY_THRESHOLDto detect when title is at sticky position.stuckclass adds visual changes via CSS (hides subtitle, hides edit button via ID, smaller title, bottom shadow)EDIT_TITLE_BUTTON_IDconstant in constants.ts for proper element identification.title .detailshasflex: 1to ensure proper flex layout within sticky containerNote: After multiple attempts with IntersectionObserver configurations proved unreliable in the VS Code webview environment, the final implementation uses direct scroll-based position detection which is more deterministic and reliable.
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.