[WEB-2509] feat: fullscreen option for editor images #5665
Conversation
|
Warning Rate limit exceeded@aaryan610 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 10 minutes and 20 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the 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. WalkthroughThe changes introduce a new toolbar component, Changes
Possibly related PRs
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: 1
Outside diff range and nitpick comments (1)
packages/editor/src/core/extensions/custom-image/components/toolbar/full-screen.tsx (1)
35-42: Consider usingwindow.open()to open the image in a new tab.Instead of creating a temporary link element, appending it to the document body, triggering a click event, and then removing the link element, you can use the
window.open()method to open the image URL directly in a new tab. Here's an example:const handleOpenInNewTab = () => { window.open(src, "_blank"); };This approach is more straightforward and eliminates the need for manipulating the DOM.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- packages/editor/src/core/extensions/custom-image/components/image-block.tsx (2 hunks)
- packages/editor/src/core/extensions/custom-image/components/index.ts (1 hunks)
- packages/editor/src/core/extensions/custom-image/components/toolbar/full-screen.tsx (1 hunks)
- packages/editor/src/core/extensions/custom-image/components/toolbar/index.ts (1 hunks)
- packages/editor/src/core/extensions/custom-image/components/toolbar/root.tsx (1 hunks)
Additional comments not posted (9)
packages/editor/src/core/extensions/custom-image/components/toolbar/index.ts (1)
1-1: More context needed to assess relevance to PR objectives.The
export * from "./root";statement is a standard way to re-export all named exports from the./rootmodule, creating a single entry point for the directory.However, without more information about the contents of the
./rootmodule and how it relates to the full-screen image feature mentioned in the PR objectives, it's difficult to determine the impact and relevance of this change.Please provide additional context or clarify how this re-export contributes to the implementation of the full-screen image functionality.
packages/editor/src/core/extensions/custom-image/components/index.ts (1)
1-1: LGTM!The new export statement is syntactically correct and aligns with the PR objective of introducing a full-screen option for editor images. The change allows other parts of the application to import the
toolbarmodule from this index file, which likely contains the UI components for the new feature. The change does not appear to have any negative impact on the existing exports.packages/editor/src/core/extensions/custom-image/components/toolbar/root.tsx (1)
1-36: LGTM!The
ImageToolbarRootcomponent follows React best practices and implements the full-screen functionality for images in a clean and readable manner. The usage of hooks, destructuring, and conditional styling enhances code clarity and maintainability.The component effectively integrates with the
ImageFullScreenActioncomponent, passing the necessary props to handle the full-screen mode. ThetoggleFullScreenModecallback allows for seamless state updates.Overall, the implementation aligns with the PR objectives and introduces the desired full-screen feature for editor images.
packages/editor/src/core/extensions/custom-image/components/toolbar/full-screen.tsx (5)
1-5: LGTM!The imports are correctly specified and follow the proper syntax. The custom helper function
cnis imported from a relative path, indicating it's a project-specific utility.
6-14: LGTM!The
Propstype correctly defines the expected properties for the component, including theimageobject with appropriate property types, theisOpenboolean to indicate the full-screen mode state, and thetoggleFullScreenModefunction to toggle the full-screen mode.
16-16: LGTM!The
MAGNIFICATION_VALUESarray contains a set of predefined magnification levels that provide a good range of options for zooming the image.
18-149: LGTM!The
ImageFullScreenActioncomponent is well-structured and implements the full-screen functionality correctly. Here are the key points:
- The component correctly handles the props and uses them appropriately.
- The state management for the magnification level is implemented correctly.
- The derived values for width, height, and aspect ratio are calculated accurately.
- The event handlers are defined using
useCallbackfor performance optimization.- The keydown event listener is properly registered and removed based on the full-screen mode state.
- The full-screen overlay is rendered conditionally based on the
isOpenprop.- The image is displayed with the correct magnification level and aspect ratio.
- The magnification controls allow increasing and decreasing the magnification level within the defined range.
- The close button and keydown event handling enable closing the full-screen mode.
- The button to toggle the full-screen mode is rendered and triggers the
toggleFullScreenModefunction when clicked.Overall, the component fulfills its purpose of providing a full-screen view of the image with magnification controls and proper event handling.
67-73: LGTM!The
useEffecthook is used correctly to register and remove the keydown event listener. ThehandleKeyDownfunction is added as the event listener when the component mounts, and the cleanup function ensures that the event listener is removed when the component unmounts, preventing any potential memory leaks. ThehandleKeyDownfunction is also included in the dependency array, ensuring that the event listener is updated if the function changes.packages/editor/src/core/extensions/custom-image/components/image-block.tsx (1)
157-164: Excellent addition of theImageToolbarRootcomponent!The introduction of the
ImageToolbarRootcomponent within theCustomImageBlockis a great enhancement. It provides a toolbar for user interactions related to the image block. The positioning and styling of the toolbar ensure a smooth user experience, as it appears on top of the image when hovered, without obstructing the view.Passing the
imageprop to theImageToolbarRootcomponent is also a smart move, as it allows the toolbar to have access to the necessary image attributes for potential modifications or actions.Overall, this addition improves the functionality and user experience of the
CustomImageBlockcomponent.
| useEffect(() => { | ||
| if (!isFullScreenEnabled) { | ||
| document.removeEventListener("keydown", handleKeyDown); | ||
| } | ||
| }, [handleKeyDown]); |
There was a problem hiding this comment.
Remove the redundant useEffect hook.
The useEffect hook at lines 75-79 is redundant and can be removed. The previous useEffect hook (lines 67-73) already handles the registration and removal of the keydown event listener based on the component's lifecycle.
Removing the event listener again when isFullScreenEnabled is false is unnecessary since it is already handled by the cleanup function in the previous useEffect hook.
Additionally, including the handleKeyDown function in the dependency array of this useEffect hook is not needed since the event listener is being removed unconditionally.
What's new?
Editor images can now be opened in a full-screen mode. A much requested feature.
Media:
Screen.Recording.2024-09-20.at.14.45.19.mov
GitHub issue: #4458
Plane issue: WEB-2509
Summary by CodeRabbit
Release Notes
ImageToolbarRootfor enhanced image interaction with a dedicated toolbar.ImageFullScreenActionfor a full-screen image viewing experience with zoom capabilities.These updates enhance user experience by providing better image management and viewing options within the application.