feat: Anvil Widget Name Component #33062
Merged
Conversation
This file contains 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
…o feat/anvil-widget-name-component
github-actions
bot
added
Anvil Pod
WalkthroughWalkthroughThe recent changes focus on enhancing the Anvil editor by introducing new components and utilities dedicated to managing and displaying widget names. These updates bring improvements such as floating UI elements, toggle functionalities for parent selection and error navigation, color application based on widget type and error state, and ensuring components stay within canvas boundaries. These changes aim to enhance user interaction and improve the identification and management of widgets. Changes
Assessment against linked issues
Recent Review DetailsConfiguration used: CodeRabbit UI Files selected for processing (1)
Files skipped from review as they are similar to previous changes (1)
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 (
|
marks0351
suggested changes
May 1, 2024
app/client/src/layoutSystems/anvil/editor/widgetName/Component.tsx
Outdated
Show resolved
Hide resolved
app/client/src/layoutSystems/anvil/editor/widgetName/Component.tsx
Outdated
Show resolved
Hide resolved
app/client/src/layoutSystems/anvil/editor/widgetName/Component.tsx
Outdated
Show resolved
Hide resolved
app/client/src/layoutSystems/anvil/editor/widgetName/SplitButton.tsx
Outdated
Show resolved
Hide resolved
marks0351
previously approved these changes
May 1, 2024
## Description A selector that provides the state of the widget name component to show. - "focus" -> Focused state - "select" -> Selected state - "none" -> Hidden ## Automation /ok-to-test tags="" ### 🔍 Cypress test results <!-- This is an auto-generated comment: Cypress test results --> > [!CAUTION] > If you modify the content in this section, you are likely to disrupt the CI result for your PR. <!-- end of auto-generated comment: Cypress test results --> ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [x] No
marks0351
previously approved these changes
May 1, 2024
riodeuno
commented
May 1, 2024
app/client/src/layoutSystems/anvil/editor/widgetName/SplitButton.tsx
Outdated
Show resolved
Hide resolved
riodeuno
commented
May 1, 2024
app/client/src/layoutSystems/anvil/editor/widgetName/Component.tsx
Outdated
Show resolved
Hide resolved
riodeuno
commented
May 1, 2024
app/client/src/layoutSystems/anvil/editor/widgetName/Component.tsx
Outdated
Show resolved
Hide resolved
riodeuno
commented
May 1, 2024
…o feat/anvil-widget-name-component
KelvinOm
reviewed
May 1, 2024
app/client/src/layoutSystems/anvil/editor/widgetName/Component.tsx
Outdated
Show resolved
Hide resolved
app/client/src/layoutSystems/anvil/editor/widgetName/Component.tsx
Outdated
Show resolved
Hide resolved
app/client/src/layoutSystems/anvil/editor/widgetName/Component.tsx
Outdated
Show resolved
Hide resolved
app/client/src/layoutSystems/anvil/editor/widgetName/Component.tsx
Outdated
Show resolved
Hide resolved
app/client/src/layoutSystems/anvil/editor/widgetName/Component.tsx
Outdated
Show resolved
Hide resolved
app/client/src/layoutSystems/anvil/editor/widgetName/Component.tsx
Outdated
Show resolved
Hide resolved
app/client/src/layoutSystems/anvil/editor/widgetName/SplitButton.tsx
Outdated
Show resolved
Hide resolved
app/client/src/layoutSystems/anvil/editor/widgetName/SplitButton.tsx
Outdated
Show resolved
Hide resolved
app/client/src/layoutSystems/anvil/editor/widgetName/SplitButton.tsx
Outdated
Show resolved
Hide resolved
…o feat/anvil-widget-name-component
riodeuno
commented
May 2, 2024
app/client/src/layoutSystems/anvil/editor/canvas/styles/anvilEditorVariables.css
Outdated
Show resolved
Hide resolved
## Description - The new widget selection UX. - This PR adjusts the code and enables the UX for Anvil. - Other things changed in this PR are as follows - [x] Update widget selection handlers to make them simplified - [x] Add `debugWidget` action for widget error debugging - [x] Add appropriate border styles to widgets - [x] Convert SVGs into react components - [x] Remove extra props from AnvilWidgetNameComponent - [x] Update and adjust CSS variable names and references Fixes #33130 33130 ## Automation /ok-to-test tags="@tag.All" ### 🔍 Cypress test results <!-- This is an auto-generated comment: Cypress test results --> > [!CAUTION] > If you modify the content in this section, you are likely to disrupt the CI result for your PR. <!-- end of auto-generated comment: Cypress test results --> ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [x] No
marks0351
approved these changes
May 2, 2024
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Out of diff range and nitpick comments (3)
app/client/src/layoutSystems/anvil/editor/AnvilEditorDetachedWidgetOnion.tsx (1)
26-26: Ensure the new hookuseAddBordersToDetachedWidgetsis well-documented.It would be beneficial to add a comment explaining the purpose and usage of the
useAddBordersToDetachedWidgetshook to maintain code readability and ease future maintenance.app/client/src/layoutSystems/anvil/editor/hooks/useAnvilWidgetStyles.ts (1)
Line range hint
12-21: Clarify the purpose of new parameters inuseAnvilWidgetStyles.Consider adding comments to explain the purpose of the new parameters (
widgetTypeandref) in theuseAnvilWidgetStyleshook. This will enhance code readability and help future developers understand the context and usage of these parameters.app/client/src/layoutSystems/common/mainContainerResizer/MainContainerResizer.tsx (1)
17-17: Ensure consistent use of CSS units.The
z-indexproperty is using a CSS variable--on-canvas-ui-zindex. It's good practice to ensure that this variable is defined and consistently used across the project for z-index values.
| @@ -14,7 +14,7 @@ const CanvasResizerIcon = importSvg( | |||
|
|
|||
| const AutoLayoutCanvasResizer = styled.div` | |||
| position: relative; | |||
| z-index: var(--ads-on-canvas-ui-zindex); | |||
| z-index: var(--on-canvas-ui-zindex); | |||
There was a problem hiding this comment.
Remove duplicate CSS transitions.
The CSS properties for transition are duplicated for width and background. This can lead to unintended behaviors or performance issues. Consider consolidating them into a single transition property.
- transition: width 300ms ease;
- transition: background 300ms ease;
+ transition: width 300ms ease, background 300ms ease;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.
Suggested change
| z-index: var(--on-canvas-ui-zindex); | |
| z-index: var(--on-canvas-ui-zindex); | |
| transition: width 300ms ease, background 300ms ease; |
Comment on lines
+24
to
+84
| export function _AnvilWidgetNameComponent( | ||
| props: { | ||
| name: string; | ||
| widgetId: string; | ||
| parentId?: string; | ||
| selectionBGCSSVar: string; | ||
| selectionColorCSSVar: string; | ||
| bGCSSVar: string; | ||
| colorCSSVar: string; | ||
| disableParentSelection: boolean; | ||
| onDragStart: React.DragEventHandler; | ||
| showError: boolean; | ||
| }, | ||
| ref: ForwardedRef<HTMLDivElement>, | ||
| ) { | ||
| const { parentId } = props; | ||
|
|
||
| /** Widget Selection Handlers */ | ||
| const { selectWidget } = useWidgetSelection(); | ||
| const handleSelectParent = useCallback(() => { | ||
| parentId && selectWidget(SelectionRequestType.One, [parentId]); | ||
| }, [parentId]); | ||
|
|
||
| const handleSelectWidget = useCallback(() => { | ||
| selectWidget(SelectionRequestType.One, [props.widgetId]); | ||
| }, [props.widgetId]); | ||
|
|
||
| const handleDebugClick = useCallback(() => { | ||
| debugWidget(props.widgetId); | ||
| }, [props.widgetId]); | ||
| /** EO Widget Selection Handlers */ | ||
|
|
||
| const leftToggle = useMemo(() => { | ||
| return { | ||
| disable: props.disableParentSelection, | ||
| onClick: handleSelectParent, | ||
| title: createMessage(ANVIL_WIDGET_NAME_TOGGLE_PARENT), | ||
| }; | ||
| }, [props.disableParentSelection, handleSelectParent]); | ||
|
|
||
| const rightToggle = useMemo(() => { | ||
| return { | ||
| disable: !props.showError, | ||
| onClick: handleDebugClick, | ||
| title: createMessage(ANVIL_WIDGET_NAME_DEBUG_CLICK), | ||
| }; | ||
| }, [props.showError, handleDebugClick]); | ||
|
|
||
| return ( | ||
| <SplitButton | ||
| bGCSSVar={props.bGCSSVar} | ||
| colorCSSVar={props.colorCSSVar} | ||
| leftToggle={leftToggle} | ||
| onClick={handleSelectWidget} | ||
| onDragStart={props.onDragStart} | ||
| ref={ref} | ||
| rightToggle={rightToggle} | ||
| text={props.name} | ||
| /> | ||
| ); | ||
| } |
There was a problem hiding this comment.
Review the use of useCallback for event handlers.
- const handleSelectParent = useCallback(() => {
- parentId && selectWidget(SelectionRequestType.One, [parentId]);
- }, [parentId]);
- const handleSelectWidget = useCallback(() => {
- selectWidget(SelectionRequestType.One, [props.widgetId]);
- }, [props.widgetId]);
- const handleDebugClick = useCallback(() => {
- debugWidget(props.widgetId);
- }, [props.widgetId]);
+ // Simplified without unnecessary useCallback wrapping
+ function handleSelectParent() {
+ if (parentId) {
+ selectWidget(SelectionRequestType.One, [parentId]);
+ }
+ }
+ function handleSelectWidget() {
+ selectWidget(SelectionRequestType.One, [props.widgetId]);
+ }
+ function handleDebugClick() {
+ debugWidget(props.widgetId);
+ }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.
Suggested change
| export function _AnvilWidgetNameComponent( | |
| props: { | |
| name: string; | |
| widgetId: string; | |
| parentId?: string; | |
| selectionBGCSSVar: string; | |
| selectionColorCSSVar: string; | |
| bGCSSVar: string; | |
| colorCSSVar: string; | |
| disableParentSelection: boolean; | |
| onDragStart: React.DragEventHandler; | |
| showError: boolean; | |
| }, | |
| ref: ForwardedRef<HTMLDivElement>, | |
| ) { | |
| const { parentId } = props; | |
| /** Widget Selection Handlers */ | |
| const { selectWidget } = useWidgetSelection(); | |
| const handleSelectParent = useCallback(() => { | |
| parentId && selectWidget(SelectionRequestType.One, [parentId]); | |
| }, [parentId]); | |
| const handleSelectWidget = useCallback(() => { | |
| selectWidget(SelectionRequestType.One, [props.widgetId]); | |
| }, [props.widgetId]); | |
| const handleDebugClick = useCallback(() => { | |
| debugWidget(props.widgetId); | |
| }, [props.widgetId]); | |
| /** EO Widget Selection Handlers */ | |
| const leftToggle = useMemo(() => { | |
| return { | |
| disable: props.disableParentSelection, | |
| onClick: handleSelectParent, | |
| title: createMessage(ANVIL_WIDGET_NAME_TOGGLE_PARENT), | |
| }; | |
| }, [props.disableParentSelection, handleSelectParent]); | |
| const rightToggle = useMemo(() => { | |
| return { | |
| disable: !props.showError, | |
| onClick: handleDebugClick, | |
| title: createMessage(ANVIL_WIDGET_NAME_DEBUG_CLICK), | |
| }; | |
| }, [props.showError, handleDebugClick]); | |
| return ( | |
| <SplitButton | |
| bGCSSVar={props.bGCSSVar} | |
| colorCSSVar={props.colorCSSVar} | |
| leftToggle={leftToggle} | |
| onClick={handleSelectWidget} | |
| onDragStart={props.onDragStart} | |
| ref={ref} | |
| rightToggle={rightToggle} | |
| text={props.name} | |
| /> | |
| ); | |
| } | |
| export function _AnvilWidgetNameComponent( | |
| props: { | |
| name: string; | |
| widgetId: string; | |
| parentId?: string; | |
| selectionBGCSSVar: string; | |
| selectionColorCSSVar: string; | |
| bGCSSVar: string; | |
| colorCSSVar: string; | |
| disableParentSelection: boolean; | |
| onDragStart: React.DragEventHandler; | |
| showError: boolean; | |
| }, | |
| ref: ForwardedRef<HTMLDivElement>, | |
| ) { | |
| const { parentId } = props; | |
| /** Widget Selection Handlers */ | |
| const { selectWidget } = useWidgetSelection(); | |
| // Simplified without unnecessary useCallback wrapping | |
| function handleSelectParent() { | |
| if (parentId) { | |
| selectWidget(SelectionRequestType.One, [parentId]); | |
| } | |
| } | |
| function handleSelectWidget() { | |
| selectWidget(SelectionRequestType.One, [props.widgetId]); | |
| } | |
| function handleDebugClick() { | |
| debugWidget(props.widgetId); | |
| } | |
| /** EO Widget Selection Handlers */ | |
| const leftToggle = useMemo(() => { | |
| return { | |
| disable: props.disableParentSelection, | |
| onClick: handleSelectParent, | |
| title: createMessage(ANVIL_WIDGET_NAME_TOGGLE_PARENT), | |
| }; | |
| }, [props.disableParentSelection, handleSelectParent]); | |
| const rightToggle = useMemo(() => { | |
| return { | |
| disable: !props.showError, | |
| onClick: handleDebugClick, | |
| title: createMessage(ANVIL_WIDGET_NAME_DEBUG_CLICK), | |
| }; | |
| }, [props.showError, handleDebugClick]); | |
| return ( | |
| <SplitButton | |
| bGCSSVar={props.bGCSSVar} | |
| colorCSSVar={props.colorCSSVar} | |
| leftToggle={leftToggle} | |
| onClick={handleSelectWidget} | |
| onDragStart={props.onDragStart} | |
| ref={ref} | |
| rightToggle={rightToggle} | |
| text={props.name} | |
| /> | |
| ); | |
| } |
riodeuno
added
ok-to-test
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/8922646970. |
|
Deploy-Preview-URL: https://ce-33062.dp.appsmith.com |
KelvinOm
approved these changes
May 2, 2024
…o feat/anvil-widget-name-component
riodeuno
added
ok-to-test
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.
Description
Widget name component and associated utilities. Feature reference: https://ce-32868.dp.appsmith.com/
Product and engineering requirements are listed in the issue description.
Fixes [Task]: Widget Name Component for Anvil #33074
Automation
/ok-to-test tags="@tag.Datasource"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/8936163355
Commit: 879a883
Cypress dashboard url: Click here!
Communication
Should the DevRel and Marketing teams inform users about this change?