feat: fullscreen #102
feat: fullscreen #102
Conversation
WalkthroughThe changes in this pull request introduce a fullscreen feature to the Fluent Editor by modifying the toolbar configuration to include a 'fullscreen' option. Additionally, new SCSS functions and mixins for handling CSS variables are added, along with updates to the styling of various components, including the editor and toolbar. The Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
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: 11
🧹 Outside diff range and nitpick comments (6)
packages/fluent-editor/src/assets/style.scss (1)
10-10: LGTM! Consider adding documentation.The addition of variable.scss import is correctly placed before component styles to ensure variable availability. Consider adding a comment to document the purpose of variable.scss and its key variables.
@import 'quill/dist/quill.snow'; +// Import global variables (includes --editor-bg-color for theming) @import './variable.scss';packages/fluent-editor/src/assets/fullscreen.scss (1)
Line range hint
1-11: Use theme variable for fullscreen background color.The browser-specific fullscreen selectors use a hardcoded white background color, while the new fullscreen class uses
getCssVar(editor-bg-color). For consistency, consider using the same theme variable.:-webkit-full-screen { - background-color: white !important; + background-color: var(--editor-bg-color) !important; } :-moz-full-screen { - background-color: white !important; + background-color: var(--editor-bg-color) !important; } :-ms-fullscreen { - background-color: white !important; + background-color: var(--editor-bg-color) !important; } :fullscreen { - background-color: white !important; + background-color: var(--editor-bg-color) !important; }packages/fluent-editor/src/fullscreen/handler.ts (1)
3-3: Consider scoping the event handler variableUsing a global variable for
exitEscHandlerBindToolbarcould cause issues when multiple editor instances are present. Consider moving this into a class property or using a WeakMap to associate handlers with specific toolbar instances.Example using WeakMap:
const exitHandlers = new WeakMap<FluentEditorToolbar, (e: KeyboardEvent) => void>();packages/fluent-editor/src/assets/common.scss (1)
281-283: Add documentation for the CSS variable getter function.The function implementation is clean and follows best practices. However, consider adding documentation to explain its usage and any naming conventions for the variables.
Add JSDoc-style documentation:
+/** + * Gets a CSS variable value by name + * @param {string} $name - The name of the CSS variable (without the -- prefix) + * @return {string} The var() expression for the CSS variable + * @example + * color: getCssVar(editor-bg-color); + */ @function getCssVar($name) { @return var(--#{$name}); }packages/fluent-editor/src/fluent-editor.ts (1)
27-31: Consider adding cleanup for fullscreen state.While the
isFullscreenproperty tracks the state correctly, consider adding cleanup logic in a destructor or disconnect method to ensure the fullscreen state is properly reset when the editor is destroyed.export class FluentEditor extends Quill { isFullscreen: boolean = false constructor(container: HTMLElement | string, options: IEditorConfig = {}) { super(container, options) } + destroy() { + if (this.isFullscreen) { + // Exit fullscreen if active + document.exitFullscreen?.() + } + super.destroy() + } }packages/fluent-editor/src/assets/toolbar.scss (1)
30-30: Document theme compatibility.Since the toolbar now uses a dynamic background color, consider adding a comment explaining the theming behavior for future maintainers.
&.ql-snow { + // Uses dynamic background color for theme compatibility (light/dark mode) background-color: getCssVar(editor-bg-color);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (11)
- packages/docs/fluent-editor/demos/custom-toolbar.vue (1 hunks)
- packages/fluent-editor/src/assets/common.scss (1 hunks)
- packages/fluent-editor/src/assets/editor.scss (1 hunks)
- packages/fluent-editor/src/assets/fullscreen.scss (1 hunks)
- packages/fluent-editor/src/assets/style.scss (1 hunks)
- packages/fluent-editor/src/assets/toolbar.scss (1 hunks)
- packages/fluent-editor/src/assets/variable.scss (1 hunks)
- packages/fluent-editor/src/config/types/editor-toolbat.interface.ts (1 hunks)
- packages/fluent-editor/src/custom-image/BlotFormatter.ts (0 hunks)
- packages/fluent-editor/src/fluent-editor.ts (3 hunks)
- packages/fluent-editor/src/fullscreen/handler.ts (1 hunks)
💤 Files with no reviewable changes (1)
- packages/fluent-editor/src/custom-image/BlotFormatter.ts
✅ Files skipped from review due to trivial changes (1)
- packages/fluent-editor/src/assets/variable.scss
🔇 Additional comments (7)
packages/fluent-editor/src/config/types/editor-toolbat.interface.ts (1)
1-6: LGTM! The interface implementation looks correct.The
FluentEditorToolbarinterface properly extends Quill'sToolbartype and adds type safety for thequillproperty, which is necessary for the fullscreen functionality.packages/fluent-editor/src/assets/style.scss (1)
10-10: Verify the variable.scss file and its contents.Let's ensure the variable.scss file exists and contains the expected CSS custom properties.
✅ Verification successful
The variable.scss file exists and contains proper CSS custom properties
The file defines the
--editor-bg-colorCSS variable with appropriate values for both light (#ffffff) and dark (#161618) themes using the :root selector.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify variable.scss exists and contains required CSS variables # Test 1: Check if variable.scss exists fd -p "packages/fluent-editor/src/assets/variable.scss$" # Test 2: Check for the --editor-bg-color variable definition rg --type scss "editor-bg-color" -A 2 -B 2Length of output: 178
Script:
#!/bin/bash # Let's check the contents of variable.scss directly cat packages/fluent-editor/src/assets/variable.scss # Also search for any CSS variable definitions without file type restriction rg "\-\-editor" -A 2 -B 2Length of output: 602
packages/fluent-editor/src/assets/fullscreen.scss (1)
22-42: Verify fullscreen toggle behavior.The implementation appears to meet the requirements from issue #44. However, let's verify the integration with the editor's toolbar configuration.
✅ Verification successful
Fullscreen implementation is properly integrated and functional
The verification confirms that:
- The fullscreen styles are correctly imported in the main
style.scss- The fullscreen handler properly manages the toggle state, class additions/removals, and ESC key functionality
- The implementation aligns with the fullscreen module interface
- The z-index values in the SCSS (51 and 50) are appropriate for the toolbar and container layering
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the fullscreen toggle implementation in the toolbar configuration # Check if the fullscreen option is properly configured in the toolbar rg -l "fullscreen.*toolbar" --type vue # Verify that the SCSS file is properly imported rg -l "import.*fullscreen\.scss" --type typescript --type vueLength of output: 167
Script:
#!/bin/bash # Search for fullscreen-related configurations in all files rg -l "fullscreen" # Search for toolbar-related configurations rg -l "toolbar" # Find all scss imports rg -l "\.scss" # Find all potential Vue components fd -e vue # Find all TypeScript files fd -e tsLength of output: 9454
Script:
#!/bin/bash # Check the fullscreen handler implementation cat packages/fluent-editor/src/fullscreen/handler.ts # Check the toolbar configuration for fullscreen integration cat packages/fluent-editor/src/toolbar/index.ts # Check if fullscreen styles are imported in the main style file cat packages/fluent-editor/src/assets/style.scss # Check the editor configuration types for fullscreen cat packages/fluent-editor/src/config/types/fullscreen-module.interface.tsLength of output: 8333
packages/docs/fluent-editor/demos/custom-toolbar.vue (1)
32-32: Verify fullscreen module initialization and update documentation.While the fullscreen option has been added to the toolbar, there are a few concerns to address:
- The fullscreen module might need initialization in the editor's modules configuration.
- As this is a demo file, consider adding example usage comments to help users understand how to properly implement the fullscreen feature.
Let's verify if the fullscreen module needs initialization:
Consider adding documentation comments above the toolbar configuration to explain the fullscreen feature usage:
const TOOLBAR_CONFIG = [ ['undo', 'redo', 'clean', 'format-painter'], [{ header: [1, 2, 3, 4, 5, 6, false] }, { font: ['songti', 'yahei', 'kaiti', 'heiti', 'lishu', 'mono', 'arial', 'arialblack', 'comic', 'impact', 'times'] }, { size: ['12px', '14px', '16px', '18px', '20px', '24px', '32px', '36px', '48px', '72px'] }], ['bold', 'italic', 'strike', 'underline'], [{ color: [] }, { background: [] }], [{ align: [] }, { list: 'ordered' }, { list: 'bullet' }, { list: 'check' }], [{ script: 'sub' }, { script: 'super' }], [{ indent: '-1' }, { indent: '+1' }], [{ direction: 'rtl' }], ['link', 'blockquote', 'code', 'code-block'], ['image', 'file', 'better-table'], + // The fullscreen option allows users to toggle fullscreen mode + // Usage: Click the fullscreen button in the toolbar to enter/exit fullscreen mode ['emoji', 'video', 'formula', 'screenshot', 'fullscreen'], ]packages/fluent-editor/src/fluent-editor.ts (1)
14-14: Implementation looks good overall!The fullscreen feature implementation follows good practices:
- Clear separation of concerns with a dedicated handler
- Proper state management via
isFullscreenproperty- Clean integration with the existing toolbar system
- No breaking changes introduced
The changes align well with the PR objectives to add fullscreen functionality.
Also applies to: 27-28, 70-70
packages/fluent-editor/src/assets/editor.scss (1)
9-9: Verify the background color variable and contrast.The background color implementation using CSS variables is a good approach for maintaining consistent theming with the new fullscreen feature.
Let's verify the CSS variable definition and usage:
Consider testing the background color contrast with foreground text colors across different themes to ensure WCAG compliance.
✅ Verification successful
Background color implementation is properly defined and consistent
The CSS variable
editor-bg-coloris well-defined invariable.scsswith appropriate values for both light (#ffffff) and dark (#161618) themes. The variable is consistently used across related files (editor.scss, toolbar.scss, fullscreen.scss) ensuring a cohesive theming experience. The contrast ratios with these background colors should work well for accessibility.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if the editor-bg-color variable is properly defined rg -l "editor-bg-color" --type scss # Look for any existing background color definitions that might conflict rg "background-color.*editor" --type scssLength of output: 137
Script:
#!/bin/bash # Search for editor-bg-color variable definition in all files rg "editor-bg-color" # Look for any background-color definitions related to editor rg "background-color.*editor" # Find all SCSS/CSS files that might contain the variable fd "\.(scss|css)$"Length of output: 1734
packages/fluent-editor/src/assets/toolbar.scss (1)
30-30: LGTM! Verify CSS variable definition.The dynamic background color addition is appropriate for theme support. However, let's verify the CSS variable exists.
✅ Verification successful
CSS variable is properly defined and consistently used
The
--editor-bg-colorCSS variable is well-defined invariable.scsswith both light (#ffffff) and dark (#161618) theme values. It's also consistently used across multiple components (editor, toolbar, fullscreen) using thegetCssVar()function, ensuring proper theme support.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the CSS variable definition exists # Expected: Find the definition of --editor-bg-color in variable.scss # Search for the CSS variable definition rg --type scss "editor-bg-color" -A 2Length of output: 66
Script:
#!/bin/bash # Search for CSS variable definition using ripgrep without file type filter rg "editor-bg-color" -A 2 # Also search for any scss/css files to understand the correct extension fd "\.(scss|css)$"Length of output: 1932
packages/fluent-editor/src/config/types/editor-toolbat.interface.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (4)
packages/fluent-editor/src/config/types/editor-toolbar.interface.ts (1)
4-6: Add JSDoc documentation to improve code clarity.Consider adding JSDoc documentation to explain the purpose of this interface and its relationship with the fullscreen feature.
+/** + * Extended toolbar interface that provides access to the FluentEditor instance. + * This extension enables advanced toolbar functionality including fullscreen mode. + */ export interface FluentEditorToolbar extends Toolbar { quill: FluentEditor }packages/fluent-editor/src/assets/fullscreen.scss (1)
43-44: Consider using viewport units for more robust height calculation.The current height calculation might not account for all edge cases. Using viewport units could provide more reliable fullscreen behavior.
-height: calc(100% - getCssVar(top)); +height: calc(100vh - getCssVar(top));packages/fluent-editor/src/config/index.ts (1)
172-173: Enhance documentation for the namespace constant.The namespace constant is well-placed in the config file and follows good practices for CSS scoping. However, the documentation could be more descriptive.
Consider expanding the documentation to better explain its purpose and usage:
-/** css namespace */ +/** + * CSS namespace prefix for the Fluent Editor. + * Used to scope CSS classes and prevent conflicts with other styles. + * @example + * // Usage in SCSS: .#{$namespace}-fullscreen { ... } + * // Renders as: .fe-fullscreen { ... } + */ export const namespace = 'fe'packages/fluent-editor/src/assets/common.scss (1)
281-281: Consider using a more descriptive namespace.The namespace 'fe' might be too short and ambiguous. Consider using a more descriptive prefix like 'fluent-editor' to prevent potential conflicts with other libraries.
-$namespace: fe; +$namespace: fluent-editor;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
- packages/fluent-editor/src/assets/common.scss (1 hunks)
- packages/fluent-editor/src/assets/fullscreen.scss (1 hunks)
- packages/fluent-editor/src/assets/variable.scss (1 hunks)
- packages/fluent-editor/src/config/index.ts (2 hunks)
- packages/fluent-editor/src/config/types/editor-toolbar.interface.ts (1 hunks)
- packages/fluent-editor/src/config/types/index.ts (1 hunks)
- packages/fluent-editor/src/fullscreen/handler.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/fluent-editor/src/assets/variable.scss
🔇 Additional comments (9)
packages/fluent-editor/src/config/types/editor-toolbar.interface.ts (2)
1-6: LGTM! The interface extension is well-structured.The implementation correctly extends Quill's Toolbar interface while maintaining type safety.
2-2: Verify the relative import path.Let's verify if there's a more direct path to import FluentEditor type.
✅ Verification successful
The current import path is correct and optimal
The current import path
../../fluent-editoris the most direct way to import theFluentEditortype. While the type is also re-exported throughindex.ts, using the direct path to the source file is preferred as it:
- Provides better clarity about where the type originates
- Avoids potential circular dependencies
- Maintains consistency with the module's internal structure
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for potential shorter import paths for FluentEditor # Test: Look for FluentEditor type definition locations rg -t typescript "export.*FluentEditor" --glob '!node_modules' # Test: Check for barrel files that might re-export FluentEditor fd -e ts -x grep -l "export.*FluentEditor" {} \;Length of output: 358
Script:
#!/bin/bash # Let's check the content of these files to understand the export structure # Check the main class definition echo "=== Content of fluent-editor.ts ===" cat "./packages/fluent-editor/src/fluent-editor.ts" # Check if index.ts is a barrel file echo -e "\n=== Content of index.ts ===" cat "./packages/fluent-editor/src/index.ts"Length of output: 6660
packages/fluent-editor/src/config/types/index.ts (1)
7-7: LGTM! Export addition follows consistent patterns.The new export for the editor toolbar interface is well-organized, maintains alphabetical ordering, and aligns with the existing module structure.
packages/fluent-editor/src/assets/fullscreen.scss (1)
26-32: LGTM! Good implementation of z-index management using CSS variables.The z-index implementation using
setCssVarandgetCssVarimproves maintainability and follows best practices.Also applies to: 35-38
packages/fluent-editor/src/fullscreen/handler.ts (4)
9-13: Enhance keyboard event handlingThe keyboard event handling can be improved for better cross-browser compatibility and user experience.
37-46: Improve handler function safetyThe handler function needs state transition guards and proper type definitions.
1-46: Implementation partially meets requirementsWhile the fullscreen functionality is implemented, the zoom capability requested in issue #44 is missing. The feature request specifically shows a reference UI with zoom controls.
#!/bin/bash # Verify zoom requirement in issue #44 gh issue view 44 --json title,bodyWould you like assistance in implementing the zoom functionality to complete the feature requirements?
18-27:⚠️ Potential issueAdd error handling and implement zoom functionality
The fullscreen entry function needs error handling and is missing the zoom functionality requested in issue #44.
Consider adding zoom controls as requested in the original issue:
function intoFullscreen(toolbar: FluentEditorToolbar) { + try { toolbar.quill.isFullscreen = true originScrollTop = document.documentElement.scrollTop toolbar.container.classList.add('fullscreen') toolbar.quill.container.classList.add('fullscreen') + // Add zoom controls + const zoomControls = document.createElement('div') + zoomControls.className = `${namespace}-zoom-controls` + toolbar.container.appendChild(zoomControls) cleanLock = lockScroll() resizeHandlerBindToolbar() window.addEventListener('resize', resizeHandlerBindToolbar) document.addEventListener('keydown', exitEscHandlerBindToolbar) + } catch (error) { + console.error('Failed to enter fullscreen:', error) + exitFullscreen(toolbar) + } }packages/fluent-editor/src/assets/common.scss (1)
282-288: LGTM! Clean implementation of CSS variable management.The implementation of
getCssVarandsetCssVarfollows SCSS best practices:
- Consistent naming convention
- Clear single responsibility
- Proper use of SCSS interpolation
- Namespace prefixing for variable isolation
Let's verify the usage of these functions in the codebase:
✅ Verification successful
Verified: CSS variable management functions are well utilized across the codebase
The functions
getCssVarandsetCssVarare actively used throughout the editor's SCSS files with consistent patterns:
- Used for theme variables (editor background color in light/dark modes)
- Manages z-index stacking in fullscreen mode
- Handles layout calculations and positioning
The implementation is not only clean but also proves to be practical and well-integrated within the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usage of getCssVar and setCssVar rg "getCssVar|setCssVar" --type scssLength of output: 67
Script:
#!/bin/bash # Search for usage of getCssVar and setCssVar with ripgrep, without file type filter rg "getCssVar|setCssVar" # Also search for any scss/sass files to understand the file extension pattern fd "\.(scss|sass)$"Length of output: 2308
* feat: default enable add protocol https * feat: add change default protocol option
|
@zzxming 有个小问题，全屏之后工具栏的图标需要变成收起全屏的图标。 |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (3)
packages/fluent-editor/src/config/types/editor-config.interface.ts (1)
13-13: Enhance the documentation comment.While the comment provides basic context, it could be more descriptive about the property's purpose and usage.
Consider expanding the comment to:
- // Auto protocol for link + // Automatically prepends protocol (e.g., 'http://') to links. When set to a string, uses that as the default protocol.packages/fluent-editor/src/link/modules/tooltip.ts (1)
26-28: Consider using a single source of truth for default options.The default
autoProtocolvalue is duplicated in the initial options declaration and inresolveOptions. Consider extracting it to a constant or class static property.+ private static DEFAULT_PROTOCOL = 'https'; options: { autoProtocol: string } = { - autoProtocol: 'https', + autoProtocol: Tooltip.DEFAULT_PROTOCOL, }packages/fluent-editor/src/config/icons.config.ts (1)
295-299: LGTM! Consider minor consistency improvements.The fullscreen exit icon implementation looks good and aligns with the PR objectives. However, for better consistency with other icons in the file:
Consider updating the SVG attributes to match the pattern used by other icons:
- <svg xmlns="http://www.w3.org/2000/svg" width="32" height="32" viewBox="0 0 24 24"> + <svg width="16px" height="16px" viewBox="0 0 24 24">This maintains the same dimensions as other icons (16px) while preserving the icon's appearance.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
- packages/fluent-editor/src/config/icons.config.ts (1 hunks)
- packages/fluent-editor/src/config/index.ts (4 hunks)
- packages/fluent-editor/src/config/types/editor-config.interface.ts (1 hunks)
- packages/fluent-editor/src/fluent-editor.ts (3 hunks)
- packages/fluent-editor/src/fullscreen/handler.ts (1 hunks)
- packages/fluent-editor/src/link/formats/link.ts (1 hunks)
- packages/fluent-editor/src/link/index.ts (2 hunks)
- packages/fluent-editor/src/link/modules/tooltip.ts (5 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/fluent-editor/src/config/index.ts
- packages/fluent-editor/src/fluent-editor.ts
🔇 Additional comments (7)
packages/fluent-editor/src/config/types/editor-config.interface.ts (1)
14-14: 🛠️ Refactor suggestionVerify the type change impact and document valid string values.
The type change from
booleantoboolean | stringcould affect existing code. Additionally, the valid string values that can be used should be documented.Let's verify the usage and impact:
Consider adding a type definition for valid protocols:
type Protocol = 'http://' | 'https://' | 'ftp://' | 'mailto:'; interface IEditorConfig extends QuillOptions { // Automatically prepends protocol to links. // When true: uses 'http://' // When string: uses the specified protocol (e.g., 'https://', 'mailto:') autoProtocol?: boolean | Protocol; // ... other properties }packages/fluent-editor/src/link/index.ts (2)
5-7: LGTM! Correct usage of Quill's import method.The change from
Quill.importstoQuill.importaligns with Quill's recommended way of importing modules.
20-20: Document the reason for TypeScript error suppression.The
@ts-expect-errordirective is suppressing a TypeScript error without explanation. This could mask legitimate type safety issues.Please:
- Document why this type error occurs and why it's safe to ignore
- Consider if there's a type-safe alternative to modifying the
SnowThemeprototypeLet's verify the type definition:
✅ Verification successful
Type error suppression is necessary due to Quill's type system limitations
The
@ts-expect-erroris necessary here because we're extending Quill's Snow theme prototype with a customextendToolbarimplementation, but Quill's TypeScript definitions don't properly expose the theme's prototype for extension. This is a common pattern when working with Quill's theming system.The code is safe because:
extendToolbaris a known Snow theme method that we're overriding- The implementation follows Quill's expected pattern, adding toolbar functionality
- Similar prototype extensions are used elsewhere in the codebase (e.g.,
buildPickers)Consider adding a comment explaining this TypeScript limitation:
// @ts-expect-error - Extending Quill Snow theme prototype (TypeScript doesn't expose theme prototypes)🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if SnowTheme has proper type definitions # Expected: Find type definitions or related issues # Look for SnowTheme type definitions rg -t ts "SnowTheme" # Check if there are similar patterns of @ts-expect-error usage rg "@ts-expect-error.*SnowTheme"Length of output: 760
Script:
#!/bin/bash # Let's check the context around the SnowTheme usage and type definitions # Check the Quill type definitions for SnowTheme rg -t ts "import.*snow" -A 5 -B 5 # Look at the context of where SnowTheme is modified rg -t ts "SnowTheme.prototype" -A 10 -B 5 # Check if there are any type definitions for themes in Quill ast-grep --pattern 'interface $_ extends Theme { $$$ }'Length of output: 5000
packages/fluent-editor/src/link/formats/link.ts (3)
1-5: LGTM! Improved type safety and Quill import pattern.The changes improve type safety by:
- Adding proper TypeScript types via Parchment import
- Using the recommended
Quill.import()pattern instead of accessingQuill.imports[]directly
Line range hint
26-29: LGTM! Robust URL sanitization implementation.The implementation includes strong security measures:
- Explicit protocol whitelist
- Proper URL sanitization
- Safe fallback URL for invalid cases
Also applies to: 40-43
Line range hint
33-39: Verify the usage ofthis.staticsafter property removal.The format method references
this.statics.blotName, but according to the changes, the statics property was removed. This might cause runtime errors.Let's verify the Quill blot implementation:
Consider simplifying the format method.
Consider this more concise implementation:
format(name, value) { - if (name !== this.statics.blotName || [false, null].includes(value)) { + if (name !== Link.blotName || !value) { super.format(name, value) } else { - this.domNode.setAttribute('href', Link.sanitize(value)) + this.domNode.setAttribute('href', this.constructor.sanitize(value)) } }The changes:
- Use
Link.blotNamedirectly instead of accessing via statics- Simplify the falsy check
- Use
this.constructorto ensure proper inheritance chainpackages/fluent-editor/src/link/modules/tooltip.ts (1)
Line range hint
261-288: Verify tooltip positioning in fullscreen mode.The
positionmethod calculates tooltip placement based on container bounds. Please verify that:
- The bounds calculation works correctly in fullscreen mode
- The tooltip remains visible and properly positioned when the editor is fullscreen
✅ Verification successful
Based on the search results, I can now generate the final response since I have enough information about how fullscreen mode is implemented and how it affects positioning:
Tooltip positioning is properly handled in fullscreen mode
The tooltip positioning code works correctly in fullscreen mode because:
- The
getBoundingClientRect()method returns coordinates relative to the viewport, which remains accurate in fullscreen mode- The fullscreen implementation (
packages/fluent-editor/src/fullscreen/handler.ts) preserves the editor's structure and only changes positioning context via fixed positioning and z-index- The bounds container reference used by the tooltip remains valid as the DOM hierarchy is maintained in fullscreen mode
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for fullscreen-related positioning logic rg -l "getBoundingClientRect.*fullscreen" # Check if other components handle fullscreen bounds ast-grep --pattern 'getBoundingClientRect().*(bottom|right|top|left)'Length of output: 115
Script:
#!/bin/bash # Let's search for fullscreen-related code and positioning logic more broadly rg -A 5 "fullscreen|getBoundingClientRect" packages/fluent-editor/src/ # Check for any fullscreen mode handling in the editor rg -A 5 "fullscreen.*mode|isFullscreen" packages/fluent-editor/src/ # Look for any tooltip or positioning related files fd -e ts -e js tooltip position packages/fluent-editor/src/Length of output: 46643
zzxming
dismissed
kagol’s stale review
The merge-base changed after approval.
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: close #44
What is the new behavior?
Does this PR introduce a breaking change?
Other information
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Style
Documentation