feat: add scroll container prop#1166
Open
DaffPunks wants to merge 1 commit into
Open
Conversation
Reviewer's GuideAdd support for sticky toolbars inside a custom scroll container by extending useSticky to accept a scrollContainerRef, wiring the prop through editor view/toolbar components, and documenting the behavior with a new Storybook experiment. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The
naturalTopFromContainervalue is computed only once on mount, so if the layout or container padding changes after initial render the sticky threshold will be off; consider recalculating it insideobserve()or when relevant reflow events fire. - The
scrollContainerreference andgetTargetare captured once inuseEffectOnce, so ifscrollContainerRef.currentever changes (e.g. different container or ref reassignment) listeners will still be attached to the old target; if that’s a valid use case, you may want to re-run the effect when the ref changes.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `naturalTopFromContainer` value is computed only once on mount, so if the layout or container padding changes after initial render the sticky threshold will be off; consider recalculating it inside `observe()` or when relevant reflow events fire.
- The `scrollContainer` reference and `getTarget` are captured once in `useEffectOnce`, so if `scrollContainerRef.current` ever changes (e.g. different container or ref reassignment) listeners will still be attached to the old target; if that’s a valid use case, you may want to re-run the effect when the ref changes.
## Individual Comments
### Comment 1
<location path="packages/editor/src/react-utils/useSticky.ts" line_range="60" />
<code_context>
- const refPageOffset = elemRef.current.getBoundingClientRect().top;
- const stickyOffset = parseInt(getComputedStyle(elemRef.current).top, 10);
- const stickyActive = refPageOffset <= stickyOffset;
+ const stickyOffset = parseInt(getComputedStyle(elemRef.current).top);
+
+ let stickyActive: boolean;
</code_context>
<issue_to_address>
**nitpick:** Explicitly pass a radix to `parseInt` for clarity and robustness
The previous code used `parseInt(..., 10)`; omitting the radix now relies on environment defaults and may conflict with lint rules. Restoring `10` keeps the behavior explicit and consistent across environments.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
def845c to
0cea852
Compare
0cea852 to
03e6fe7
Compare
d3m1d0v
reviewed
Jul 1, 2026
Comment on lines
+25
to
+30
| const naturalTopFromContainer = | ||
| scrollContainer && elemRef.current | ||
| ? elemRef.current.getBoundingClientRect().top - | ||
| scrollContainer.getBoundingClientRect().top - | ||
| scrollContainer.clientTop | ||
| : null; |
Member
There was a problem hiding this comment.
I think it's better to move this to the observe()
Member
There was a problem hiding this comment.
Please, move this story to demo/src/stories/examples
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
If editor placed in scrollable container, this happens:
2026-06-30.21.53.49.mov
Because isStickyActive depends on window.addEventListener('scroll', ...). When window is not scrollable, toolbar don't add class:
g-md-editor-sticky_sticky-activeNow, with scrollContainerRef if behaves like this:
https://preview.gravity-ui.com/md-editor/1166/?path=/story/experiments-sticky-toolbar-in-scroll-container--story
Summary by Sourcery
Add support for sticky toolbars that work inside a custom scroll container instead of only the browser window.
New Features:
Enhancements: