chore: remove padding for anvil embed apps#37173
Conversation
WalkthroughThe pull request introduces modifications to the Changes
Possibly related PRs
Suggested labels
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
Documentation and Community
|
github-actions
bot
added
the
skip-changelog
| import { deriveColumnHighlights } from "layoutSystems/anvil/utils/layouts/highlights/columnHighlights"; | ||
| import { MainCanvasWrapper } from "./MainCanvasWrapper"; | ||
|
|
||
| class LayoutColumn extends BaseLayoutComponent { |
There was a problem hiding this comment.
@riodeuno This type has not been used anywhere. The same situation applies to many other types (we need to check this). In this PR, I use LayoutColumn type for the main container, since previously changes to ALIGNED_LAYOUT_COLUMN affected not only the main container but also the zones
![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 (3)
app/client/src/layoutSystems/anvil/utils/AnvilDSLTransformer.ts (2)
Line range hint
1-11: Documentation needs update to reflect layout style changes.The function's documentation should mention that layout styles (padding, etc.) are intentionally excluded for embed apps.
/** * This function transforms a page's DSL to become compatible with the Anvil Layout system * Note: This also gets rid of any properties that are not needed for the Auto Layout system + * Note: Layout styles like padding are excluded for embed apps * @returns dsl: The transformed DSL */
Line range hint
12-35: Consider adding type safety for layout properties.The layout object creation uses a fixed structure. Consider defining an interface for better type safety.
interface AnvilLayout { layoutId: string; layoutType: LayoutComponentTypes; layout: Array<any>; isDropTarget: boolean; isPermanent: boolean; childTemplate: { insertChild: boolean; isDropTarget: boolean; isPermanent: boolean; layout: Array<any>; layoutId: string; layoutType: LayoutComponentTypes; maxChildLimit: number; }; }app/client/src/layoutSystems/anvil/layoutComponents/components/LayoutColumn.tsx (1)
38-44: LGTM: New view mode implementationThe MainCanvasWrapper integration aligns with the PR objective of removing padding for anvil embed apps.
Consider adding a brief JSDoc comment explaining the purpose of
renderViewModemethod.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
app/client/src/layoutSystems/anvil/layoutComponents/components/LayoutColumn.tsx(2 hunks)app/client/src/layoutSystems/anvil/layoutComponents/components/MainCanvasWrapper.tsx(1 hunks)app/client/src/layoutSystems/anvil/utils/AnvilDSLTransformer.ts(1 hunks)
🔇 Additional comments (7)
app/client/src/layoutSystems/anvil/layoutComponents/components/MainCanvasWrapper.tsx (2)
6-16: LGTM! Clean implementation of conditional padding based on embed status.
The component correctly implements the padding adjustment based on URL parameters while maintaining proper prop forwarding.
6-16: Verify test coverage for this component.
Since this component handles critical layout behavior, ensure it has proper test coverage for both embedded and non-embedded scenarios.
app/client/src/layoutSystems/anvil/utils/AnvilDSLTransformer.ts (1)
21-21: Verify the impact of changing layoutType to LAYOUT_COLUMN.
The change from ALIGNED_LAYOUT_COLUMN to LAYOUT_COLUMN could affect existing layouts.
app/client/src/layoutSystems/anvil/layoutComponents/components/LayoutColumn.tsx (4)
1-2: LGTM: Import change aligns with layout modifications
Line range hint 17-26: Verify widget row alignment compatibility
The change to ALIGNED_WIDGET_ROW should maintain compatibility with existing widget layouts.
#!/bin/bash
# Search for existing WIDGET_ROW usage
ast-grep --pattern 'layoutType: LayoutComponentTypes.WIDGET_ROW'28-36: LGTM: Flex layout properties use design system tokens
The addition of height and gap properties improves layout consistency.
#!/bin/bash
# Verify spacing token usage across the codebase
rg "spacing-4" --type ts15-15: Verify highlight behavior with aligned columns
The change from deriveColumnHighlights to deriveAlignedColumnHighlights may affect column highlighting behavior.
app/client/src/layoutSystems/anvil/layoutComponents/components/MainCanvasWrapper.tsx
Show resolved
Hide resolved
KelvinOm
force-pushed
the
chore/anvil-embed
branch
from
6f241e1 to
f15d543
Compare
|
@KelvinOm These changes will not effect existing Anvil applications, or will they? |
These changes should not affect existing apps. The embed check will not work for them. |
KelvinOm
force-pushed
the
chore/anvil-embed
branch
from
f15d543 to
4ccd877
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
app/client/src/layoutSystems/anvil/layoutComponents/components/LayoutColumn.tsx (1)
32-33: Consider documenting the spacing constant.The layout properties look good, but it would be helpful to document where the "spacing-4" constant is defined.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
app/client/src/layoutSystems/anvil/layoutComponents/components/LayoutColumn.tsx(2 hunks)app/client/src/layoutSystems/anvil/layoutComponents/components/MainCanvasWrapper.tsx(1 hunks)app/client/src/layoutSystems/anvil/utils/AnvilDSLTransformer.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- app/client/src/layoutSystems/anvil/layoutComponents/components/MainCanvasWrapper.tsx
- app/client/src/layoutSystems/anvil/utils/AnvilDSLTransformer.ts
🔇 Additional comments (3)
app/client/src/layoutSystems/anvil/layoutComponents/components/LayoutColumn.tsx (3)
24-24: LGTM! Layout type change aligns with container modifications.
The switch to ALIGNED_WIDGET_ROW is consistent with the container-specific changes mentioned in the previous discussion.
38-44: Verify MainCanvasWrapper's padding behavior in embed mode.
The new render method looks good, but given the failing Cypress tests and the PR's objective to remove padding, we should verify the wrapper's behavior.
#!/bin/bash
# Check MainCanvasWrapper implementation for padding logic
rg -A 5 "MainCanvasWrapper" --type tsx
# Check for related test files
fd -e spec.tsx -e spec.ts "MainCanvasWrapper"1-2: LGTM! Verify impact on existing highlight behavior.
The switch to aligned column highlights is consistent with the layout changes.
✅ Verification successful
Import change is safe and consistent with codebase patterns
Both highlight derivation methods coexist in the codebase with clear separation of concerns:
deriveColumnHighlightsis used byWidgetColumnderiveAlignedColumnHighlightsis used byLayoutColumn,AlignedLayoutColumn, andAlignedWidgetColumn
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any other uses of the old highlight derivation
rg "deriveColumnHighlights" --type ts
Length of output: 112
Script:
#!/bin/bash
# Search for both old and new highlight derivation methods
rg "derive.*ColumnHighlights" --type typescript -A 3
# Search for files containing Column highlights
fd -e ts -e tsx . | xargs rg "Column.*Highlights"
# Check imports in layout related files
fd -e ts -e tsx "Layout.*" | xargs rg "^import.*Highlights"
Length of output: 5632
## Description I removed the styles from the DSL and changed the layout type for main container to `LAYOUT_COLUMN`. ## Automation /ok-to-test tags="@tag.All" ### 🔍 Cypress test results <!-- This is an auto-generated comment: Cypress test results --> > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: <https://github.com/appsmithorg/appsmith/actions/runs/11720581716> > Commit: 4ccd877 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=11720581716&attempt=2" target="_blank">Cypress dashboard</a>. > Tags: `@tag.All` > Spec: > <hr>Thu, 07 Nov 2024 11:30:39 UTC <!-- end of auto-generated comment: Cypress test results --> ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [x] No <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Introduced the `MainCanvasWrapper` component for dynamic styling based on URL parameters. - Added a new rendering behavior for the `LayoutColumn` component. - **Improvements** - Updated highlight derivation and layout type in the `LayoutColumn` component. - Simplified layout structure in the `anvilDSLTransformer` function. - Adjusted layout properties for better alignment and spacing in the `LayoutColumn` component. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
3 participants
Description
I removed the styles from the DSL and changed the layout type for main container to
LAYOUT_COLUMN.Automation
/ok-to-test tags="@tag.All"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11720581716
Commit: 4ccd877
Cypress dashboard.
Tags:
@tag.AllSpec:
Thu, 07 Nov 2024 11:30:39 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
MainCanvasWrappercomponent for dynamic styling based on URL parameters.LayoutColumncomponent.Improvements
LayoutColumncomponent.anvilDSLTransformerfunction.LayoutColumncomponent.