fix: screenshot functional bug #81
Conversation
WalkthroughThe changes introduce modifications to the styling and functionality related to full-screen elements and screenshot capabilities. The Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (7)
packages/fluent-editor/src/assets/fullscreen.scss (2)
2-2: Consistent full-screen background color across browsers.The changes ensure a consistent white background for full-screen elements across different browser implementations. This is good for cross-browser compatibility.
While the use of
!importantis generally discouraged, it might be necessary in this case to override any potential conflicting styles. However, consider if there's a way to achieve the same result without!importantto maintain better CSS specificity control in the future.Also applies to: 5-5, 8-8, 11-11
13-21: New .scroll class enhances scrolling control.The addition of the
.scrollclass with its modifiers provides useful functionality for controlling scroll behavior. The use of BEM naming convention is consistent with best practices.Consider adding a comment or documentation to explain the purpose and usage of the
.scrollclass and its modifiers. This will help other developers understand when and how to use these new styles.Example:
// .scroll - Controls scrolling behavior // &--lock: Prevents scrolling (useful for modals or during transitions) // &__wrap: Allows scrolling within a full-height container .scroll { // ... (existing code) }packages/fluent-editor/src/utils/scroll-lock.ts (1)
2-25: LGTM! Consider adding error handling and performance optimization.The
getScrollBarWidthfunction is well-implemented and uses a common technique to calculate the scrollbar width. The caching mechanism is a good optimization.However, consider the following improvements:
- Add error handling for DOM operations to make the function more robust.
- Consider the performance implications of DOM manipulation, especially if this function is called frequently.
Here's a suggestion to add basic error handling:
export const getScrollBarWidth = ({ target = document.body } = {}): number => { if (scrollBarWidth !== undefined) return scrollBarWidth + try { const outer = document.createElement('div') outer.className = 'scroll__wrap' outer.style.visibility = 'hidden' outer.style.width = '100px' outer.style.position = 'absolute' outer.style.top = '-9999px' target.appendChild(outer) const widthNoScroll = outer.offsetWidth outer.style.overflow = 'scroll' const inner = document.createElement('div') inner.style.width = '100%' outer.appendChild(inner) const widthWithScroll = inner.offsetWidth outer.parentNode?.removeChild(outer) scrollBarWidth = widthNoScroll - widthWithScroll return scrollBarWidth + } catch (error) { + console.error('Error calculating scrollbar width:', error) + return 0 // fallback to 0 if calculation fails + } }This change adds a try-catch block to handle any potential errors during DOM manipulation and provides a fallback value.
packages/fluent-editor/src/toolbar/index.ts (1)
143-143: Approved: Good fix for screenshot functionality.The change to prevent scrolling when focusing for screenshots is a good solution to the reported bug. It should improve the user experience without affecting other functionality.
For improved clarity and maintainability, consider extracting the condition into a named constant:
+ const isScreenshotFormat = format === 'screenshot'; - this.quill.focus({ preventScroll: format === 'screenshot' }) + this.quill.focus({ preventScroll: isScreenshotFormat })This makes the code more readable and easier to modify if additional formats need similar treatment in the future.
packages/fluent-editor/src/screenshot/index.ts (3)
Line range hint
94-106: LGTM: Improved selection handling and scroll locking.The changes in this segment improve the screenshot functionality:
- Removing the
trueargument fromgetSelectionallows for more flexible selection handling.- Using
lockScroll()and storing its result incleanLockprovides a clear mechanism for managing scroll locking.These changes enhance the overall robustness of the screenshot process.
Consider renaming
cleanLocktounlockScrollorreleaseScrollLockfor better clarity on its purpose.
119-119: LGTM: Consistent cleanup of scroll lock.The addition of
cleanLock()after removing the wrapper is a good practice. It ensures that the scroll lock is properly released when the screenshot process is completed, maintaining consistent scroll behavior.For consistency, consider moving the
cleanLock()call right afterwrapper.remove()in both this function and theremoveContextmenufunction. This ensures the cleanup order is the same in both cases.
Line range hint
1-194: Overall: Improved screenshot functionality with better scroll management.The changes in this file significantly enhance the screenshot functionality by introducing a more robust scroll locking mechanism. The use of the
lockScrollutility and consistent cleanup improve the overall reliability of the screenshot process. The code is now more organized and follows better practices for resource management.Key improvements:
- Introduction of a dedicated
lockScrollutility.- Consistent cleanup of scroll locking across different scenarios.
- More flexible selection handling in the
Screenshotfunction.These changes should result in a more stable and predictable user experience when using the screenshot feature.
Consider adding error handling for the
lockScrollandcleanLockoperations to ensure robustness in case of unexpected issues with scroll management.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- packages/fluent-editor/src/assets/fullscreen.scss (1 hunks)
- packages/fluent-editor/src/screenshot/index.ts (3 hunks)
- packages/fluent-editor/src/toolbar/index.ts (1 hunks)
- packages/fluent-editor/src/utils/scroll-lock.ts (1 hunks)
🧰 Additional context used
Biome
packages/fluent-editor/src/utils/scroll-lock.ts
[error] 34-34: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
🔇 Additional comments not posted (5)
packages/fluent-editor/src/assets/fullscreen.scss (1)
1-21: Request for clarification on bug fix implementation.The changes made to the full-screen styles and the addition of the
.scrollclass appear to be related to the screenshot functional bug mentioned in the PR objectives. However, to ensure that these changes fully address the issue:
- Could you provide more details about the specific screenshot bug that was being experienced?
- How do these CSS changes resolve the issue?
- Have you tested the fix across different browsers to ensure consistent behavior?
This information will help in understanding the full impact of these changes and ensure that the bug has been completely resolved.
packages/fluent-editor/src/utils/scroll-lock.ts (1)
1-52: Summary of review for scroll-lock.tsOverall, this file introduces well-implemented utility functions for managing scroll behavior and scrollbar width. The main points for improvement are:
- Add error handling to
getScrollBarWidthfunction.- Fix the typo in the 'clockClass' variable name in
lockScrollfunction.- Refactor the assignment in expression in the
cleanLockfunction.- Consider using data attributes instead of classes for locking scroll.
- Optimize performance by using
requestAnimationFramefor style changes.These improvements will enhance the robustness, maintainability, and performance of the code. Great job on implementing these utility functions, and with these minor adjustments, the code will be even better!
🧰 Tools
Biome
[error] 34-34: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
packages/fluent-editor/src/screenshot/index.ts (3)
5-5: LGTM: Good practice for code organization.The addition of the
lockScrollimport from a separate utility file is a good practice. It improves code organization and promotes reusability of the scroll locking functionality.
94-94: LGTM: Improved scroll locking mechanism.The removal of the manual
overflow: hiddensetting is a good change. It's consistent with the introduction of thelockScrollutility, which likely provides a more robust and reusable solution for managing scroll behavior during the screenshot process.
111-111: LGTM: Proper cleanup of scroll lock.The addition of
cleanLock()before removing the event listener is a good practice. It ensures that the scroll lock is properly released when the screenshot process is cancelled, preventing potential issues with scroll behavior.
PR
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: N/A
What is the new behavior?
Does this PR introduce a breaking change?
Other information
Summary by CodeRabbit
New Features
.scrollclass for enhanced scrolling behavior in full-screen mode.Improvements
Bug Fixes