feat: Variable width for property pane size and side by side editor #30550
Conversation
…o pp-ee-proption
…o pp-ee-proption
WalkthroughThe changes primarily aim at enhancing user experience by making the widths of the Editor Segments and Property Pane adapt to the user's screen size. This adjustment ensures a more spacious and flexible code area, especially for users with larger screens, and improves the interface when using the application in split-screen mode. The modifications span across various components and hooks, introducing new constants for default widths, updating type definitions to accommodate flexible sizing, and refining the logic for dynamic layout adjustments based on the editor mode and device characteristics. Changes
Assessment against linked issues
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
github-actions
bot
added
IDE Navigation
|
/ok-to-test tags="@tag.All" |
|
Whoa, @tag.All spotted in your test suite! 🚀 |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/7624073436. |
|
/build-deploy-preview skip-test=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/7624082814. |
|
Deploy-Preview-URL: https://ce-30550.dp.appsmith.com |
|
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/7624073436.
|
| @@ -44,6 +46,7 @@ export const MOBILE_MAX_WIDTH = 767; | |||
| export const TABLET_MIN_WIDTH = 768; | |||
| export const TABLET_MAX_WIDTH = 991; | |||
| export const DESKTOP_MIN_WIDTH = 992; | |||
| export const DESIGN_BASE_WIDTH = 1440; | |||
There was a problem hiding this comment.
Design base width feels ambiguous. Can we call it App or Editor instead?
| export const DESIGN_BASE_WIDTH = 1440; | |
| export const EDITOR_BASE_WIDTH = 1440; |
…o pp-ee-proption
…o pp-ee-proption
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| width: | ||
| window.innerWidth > 1440 | ||
| ? DEFAULT_PP_LARGE_WIDTH | ||
| : DEFAULT_PROPERTY_PANE_WIDTH, |
There was a problem hiding this comment.
The logic for setting the width of the property pane based on the window width is straightforward and correctly implements the feature enhancement described. However, directly accessing window.innerWidth in the reducer's initial state is not ideal for server-side rendering (SSR) scenarios or testing environments where window might not be available.
Consider using a more SSR-friendly approach, such as injecting the window width into the reducer through an action dispatched on the client side after the application mounts.
|
/build-deploy-preview skip-test=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/7692411357. |
|
Deploy-Preview-URL: https://ce-30550.dp.appsmith.com |
|
/build-deploy-preview skip-test=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/7706202058. |
| } | ||
|
|
||
| const Wrapper = styled.section<{ | ||
| background: string; | ||
| width: number; | ||
| $enableMainCanvasResizer: boolean; | ||
| $scale?: number; |
There was a problem hiding this comment.
The $scale property added to the Wrapper styled component is correctly implemented, enabling CSS-based scaling of the canvas. Ensure that the scale value is always within a reasonable range to prevent UI distortion.
Consider adding validation to ensure scale is within a predefined range (e.g., 0.5 to 2) to prevent extreme scaling that could distort the UI.
| <Flex | ||
| backgroundColor="var(--ads-v2-color-bg)" | ||
| border="1px solid var(--ads-v2-color-border)" | ||
| borderRadius="var(--ads-v2-border-radius)" | ||
| bottom="var(--ads-v2-spaces-4)" | ||
| left="var(--ads-v2-spaces-5)" | ||
| position="sticky" | ||
| px="spaces-3" | ||
| py="spaces-2" | ||
| style={{ boxShadow: "var(--ads-v2-shadow-popovers)" }} | ||
| width="120px" | ||
| > | ||
| <Button | ||
| isIconButton | ||
| kind="tertiary" | ||
| onClick={() => setScale(scale - 0.1)} | ||
| size="sm" | ||
| startIcon="subtract-line" | ||
| /> | ||
| <Flex | ||
| alignItems="center" | ||
| flex="1" | ||
| justifyContent="center" | ||
| px="spaces-2" | ||
| > | ||
| {Math.round(scale * 100)}% | ||
| </Flex> | ||
| <Button | ||
| isIconButton | ||
| kind="tertiary" | ||
| onClick={() => setScale(scale + 0.1)} | ||
| size="sm" | ||
| startIcon="add-line" | ||
| /> | ||
| </Flex> |
There was a problem hiding this comment.
The implementation of UI controls for adjusting the scale state is correctly done, using Button components for incrementing and decrementing the scale value. However, consider adding bounds checking to prevent the scale from going below a minimum or above a maximum value.
Consider adding bounds checking to the setScale calls to ensure the scale value remains within a reasonable range (e.g., between 0.5 and 2) to prevent extreme scaling.
- onClick={() => setScale(scale - 0.1)}
+ onClick={() => setScale(prevScale => Math.max(0.5, prevScale - 0.1))}
- onClick={() => setScale(scale + 0.1)}
+ onClick={() => setScale(prevScale => Math.min(2, prevScale + 0.1))}Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| <Flex | |
| backgroundColor="var(--ads-v2-color-bg)" | |
| border="1px solid var(--ads-v2-color-border)" | |
| borderRadius="var(--ads-v2-border-radius)" | |
| bottom="var(--ads-v2-spaces-4)" | |
| left="var(--ads-v2-spaces-5)" | |
| position="sticky" | |
| px="spaces-3" | |
| py="spaces-2" | |
| style={{ boxShadow: "var(--ads-v2-shadow-popovers)" }} | |
| width="120px" | |
| > | |
| <Button | |
| isIconButton | |
| kind="tertiary" | |
| onClick={() => setScale(scale - 0.1)} | |
| size="sm" | |
| startIcon="subtract-line" | |
| /> | |
| <Flex | |
| alignItems="center" | |
| flex="1" | |
| justifyContent="center" | |
| px="spaces-2" | |
| > | |
| {Math.round(scale * 100)}% | |
| </Flex> | |
| <Button | |
| isIconButton | |
| kind="tertiary" | |
| onClick={() => setScale(scale + 0.1)} | |
| size="sm" | |
| startIcon="add-line" | |
| /> | |
| </Flex> | |
| <Flex | |
| backgroundColor="var(--ads-v2-color-bg)" | |
| border="1px solid var(--ads-v2-color-border)" | |
| borderRadius="var(--ads-v2-border-radius)" | |
| bottom="var(--ads-v2-spaces-4)" | |
| left="var(--ads-v2-spaces-5)" | |
| position="sticky" | |
| px="spaces-3" | |
| py="spaces-2" | |
| style={{ boxShadow: "var(--ads-v2-shadow-popovers)" }} | |
| width="120px" | |
| > | |
| <Button | |
| isIconButton | |
| kind="tertiary" | |
| onClick={() => setScale(prevScale => Math.max(0.5, prevScale - 0.1))} | |
| size="sm" | |
| startIcon="subtract-line" | |
| /> | |
| <Flex | |
| alignItems="center" | |
| flex="1" | |
| justifyContent="center" | |
| px="spaces-2" | |
| > | |
| {Math.round(scale * 100)}% | |
| </Flex> | |
| <Button | |
| isIconButton | |
| kind="tertiary" | |
| onClick={() => setScale(prevScale => Math.min(2, prevScale + 0.1))} | |
| size="sm" | |
| startIcon="add-line" | |
| /> | |
| </Flex> |
|
Deploy-Preview-URL: https://ce-30550.dp.appsmith.com |
| @@ -12,6 +12,9 @@ export const CANVAS_BACKGROUND_COLOR = "#FFFFFF"; | |||
| export const DEFAULT_ENTITY_EXPLORER_WIDTH = 256; | |||
| export const DEFAULT_PROPERTY_PANE_WIDTH = 288; | |||
| export const APP_SETTINGS_PANE_WIDTH = 525; | |||
| export const DEFAULT_APP_SIDEBAR_WIDTH = 50; | |||
| export const DEFAULT_EDITOR_PANE_WIDTH = 255; | |||
| export const DEFAULT_PP_LARGE_WIDTH = window.innerWidth * 0.404 - 255; // 40.4% - 255px | |||
There was a problem hiding this comment.
can we add comments explaining the rationale here.
| }>` | ||
| background: ${({ background }) => background}; | ||
| width: ${({ $enableMainCanvasResizer, width }) => | ||
| $enableMainCanvasResizer ? `100%` : `${width}px`}; | ||
| transform: ${({ $scale }) => ($scale ? `scale(${$scale})` : "")}; |
There was a problem hiding this comment.
pls ensure Drag and drop works properly when scaled.
There was a problem hiding this comment.
tested the DnD and I see it not working in a few scenarios like when the canvas is scaled and try to do drag and drop, coz this scale factor needs to be used for the canvas on which the DnD illusion is rendered. pls check StickyCanvasArena for more details. I remember @hetunandu implemented a similar scale factor based on a previous redesign project which involved creating a redux state for it.
Also with this scaling I assume we might need to implement horizontal auto scroll while dragging and dropping. this itself is not a deal breaker for the first cut though.
| @@ -253,6 +255,41 @@ function MainContainerWrapper(props: MainCanvasWrapperProps) { | |||
| </div> | |||
| )} | |||
| {node} | |||
| <Flex | |||
There was a problem hiding this comment.
I would recommend moving this new block into a seperate component.
There was a problem hiding this comment.
This part is just poc for now, will account for your comments if we go ahead with this changes.
…o pp-ee-proption
…o pp-ee-proption
…o pp-ee-proption
|
/build-deploy-preview skip-test=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/7737775819. |
|
Deploy-Preview-URL: https://ce-30550.dp.appsmith.com |
|
/ok-to-test tags="@tags.All" |
|
Please use |
|
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/7754543017. |
…o pp-ee-proption
|
/ok-to-test tags="@tag.All" |
|
Whoa, @tag.All spotted in your test suite! 🚀 |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/7754681425. |
|
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/7754681425.
|
|
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/7754681425.
|
Description
This PR adds adaptive width to property pane and side by side editor if screen size if bigger than 1440.
https://www.notion.so/appsmith/IDE-Solution-behaviour-8607a3b4df254970a77bd077290d3690#05be7aee530e4669ab6c79bb34ffadc7
PR fixes following issue(s)
Fixes #30086
Type of change
Testing
How Has This Been Tested?
Test Plan
Issues raised during DP testing
Checklist:
Dev activity
QA activity:
Test Plan Approvedlabel after Cypress tests were reviewedTest Plan Approvedlabel after JUnit tests were reviewedSummary by CodeRabbit
PropertyPaneWrapperto use new hooks for dynamic width management.useEditorPaneWidthhook touseIDEWidthsfor clearer purpose and functionality.