Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: add tests for anvil modal. #34347

Merged
merged 6 commits into from
Jun 24, 2024
Merged

chore: add tests for anvil modal. #34347

merged 6 commits into from
Jun 24, 2024

Conversation

marks0351
Copy link
Contributor

@marks0351 marks0351 commented Jun 19, 2024

workerB

Description

Tip

Add a TL;DR when the description is longer than 500 words or extremely technical (helps the content, marketing, and DevRel team).

Please also include relevant motivation and context. List any dependencies that are required for this change. Add links to Notion, Figma or any other documents that might be relevant to the PR.

Fixes #33740
or
Fixes Issue URL

Warning

If no issue exists, please create an issue first, and check with the maintainers if the issue is valid.

Automation

/ok-to-test tags="@tag.Anvil"

🔍 Cypress test results

Tip

🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/9643638292
Commit: bcc4bbd
Cypress dashboard.
Tags: @tag.Anvil

Communication

Should the DevRel and Marketing teams inform users about this change?

  • Yes
  • No

Summary by CodeRabbit

  • New Features

    • Introduced test cases for Anvil modals, covering interactions like opening, closing, drag and drop operations, and handling modal functions.
  • Bug Fixes

    • Enhanced testing capabilities with data-testid attributes for better identification and testing of components, particularly in detached widget drop areas.
  • Refactor

    • Updated the Modal component to use dataAttributes instead of size prop to streamline attribute handling.
    • Dynamic generation of modal class names based on properties for better styling and consistency.
  • Style

    • Adjusted styling for SVG elements within the EntityExplorer component, specifically modifying the height and width properties.

Copy link
Contributor

coderabbitai bot commented Jun 19, 2024

Walkthrough

The changes introduce new tests and functionality for Anvil modals, including interactions like opening/closing modals and handling drag-and-drop operations. The refactoring includes updates in various files to enhance testing attributes, adjust component props, and refine modal handling procedures.

Changes

Files Summary
.../ClientSide/Anvil/AnvilModal_spec.ts Added new test cases for Anvil modals, covering interactions like opening/closing, DnD operations, and handling modal functions (onClose, onSubmit).
.../support/Pages/Anvil/AnvilDnDHelper.ts Introduced dropModal property in DropTargetDetails interface, updated AnvilDnDHelper logic to handle this new property.
.../support/Pages/Anvil/Locators/index.ts Added new selectors for modal elements and a new selector for detached widgets drop arena.
.../src/layoutSystems/anvil/common/AnvilFlexComponent.tsx Included AnvilDataAttributes and widgetName to the props, updating Flex component props accordingly.
.../src/layoutSystems/anvil/editor/canvasArenas/DetachedWidgetsDropArena.tsx Added data-testid attribute for testing in DetachedWidgetsDropArena.
.../src/layoutSystems/anvil/editor/hooks/useAnvilWidgetStyles.ts Removed setting of a specific attribute on a DOM element for testing attributes.
.../src/layoutSystems/anvil/viewer/canvas/AnvilDetachedWidgets.tsx Removed usage of getAnvilWidgetDOMId and updated defaultWidgetProps.
.../src/components/Modal/src/Modal.tsx Modified Modal component to accept dataAttributes prop instead of size.
.../src/components/Modal/src/types.ts Replaced size prop with dataAttributes in ModalProps.
.../src/components/Modal/stories/ModalExamples.tsx Updated Modal examples to use dataAttributes instead of size.
.../widgets/src/testing/ComplexForm.tsx Added dataAttributes to Modal component while removing size prop.
.../src/widgets/anvil/constants.ts Added MODAL_SIZE to AnvilDataAttributes object.
.../src/widgets/wds/WDSModalWidget/widget/index.tsx Modified getModalClassNames method to dynamically generate modal class names and updated class names in rendering logic.
.../Editor/Explorer/EntityExplorer.tsx Updated styling code for SVG element's height and width properties.

Sequence Diagram(s)

No sequence diagrams are required for these changes as they are mostly updates and enhancements to existing functionalities.

Assessment against linked issues

Objective (Issue #33740) Addressed Explanation
Implement E2E tests for Anvil Modal widget

Poem

Here's to the Anvil modals, so true,
With updates fresh, and tests anew.
They open, they close, and smoothly they glide,
With data attributes now, they stand with pride.
Drag-and-drop, seamless and swift,
In the Anvil world, a delightful shift. 🌟


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?

Share
Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai generate interesting stats about this repository and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@github-actions github-actions bot added the skip-changelog Adding this label to a PR prevents it from being listed in the changelog label Jun 19, 2024
@marks0351 marks0351 requested review from riodeuno and jsartisan and removed request for ApekshaBhosale June 19, 2024 13:39
@marks0351 marks0351 added ok-to-test Required label for CI and removed skip-changelog Adding this label to a PR prevents it from being listed in the changelog labels Jun 19, 2024
@github-actions github-actions bot added Anvil Pod Issue related to Anvil project potential-duplicate This label marks issues that are potential duplicates of already open issues Task A simple Todo skip-changelog Adding this label to a PR prevents it from being listed in the changelog labels Jun 19, 2024
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Outside diff range and nitpick comments (1)
app/client/src/layoutSystems/anvil/editor/hooks/useAnvilWidgetStyles.ts (1)

Line range hint 12-12: The default parameter isVisible should follow the last required parameter to prevent confusion and potential errors at the call site.

- useAnvilWidgetStyles(widgetId, widgetName, isVisible = true, widgetType, elevatedBackground, ref)
+ useAnvilWidgetStyles(widgetId, widgetName, widgetType, elevatedBackground, ref, isVisible = true)
Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 3637246 and ba6956e.

Files ignored due to path filters (3)
  • app/client/cypress/snapshots/anvil/anvilModalLargeSize.snap.png is excluded by !**/*.png
  • app/client/cypress/snapshots/anvil/anvilModalMediumSize.snap.png is excluded by !**/*.png
  • app/client/cypress/snapshots/anvil/anvilModalSmallSize.snap.png is excluded by !**/*.png
Files selected for processing (9)
  • app/client/cypress/e2e/Regression/ClientSide/Anvil/AnvilModal_spec.ts (1 hunks)
  • app/client/cypress/support/Pages/Anvil/AnvilDnDHelper.ts (3 hunks)
  • app/client/cypress/support/Pages/Anvil/AnvilLayout.ts (1 hunks)
  • app/client/cypress/support/Pages/Anvil/Locators/index.ts (3 hunks)
  • app/client/src/layoutSystems/anvil/common/AnvilFlexComponent.tsx (3 hunks)
  • app/client/src/layoutSystems/anvil/editor/canvasArenas/DetachedWidgetsDropArena.tsx (1 hunks)
  • app/client/src/layoutSystems/anvil/editor/hooks/useAnvilWidgetStyles.ts (1 hunks)
  • app/client/src/layoutSystems/anvil/viewer/canvas/AnvilDetachedWidgets.tsx (2 hunks)
  • app/client/src/widgets/wds/WDSModalWidget/widget/index.tsx (3 hunks)
Additional context used
Biome
app/client/src/layoutSystems/anvil/editor/hooks/useAnvilWidgetStyles.ts

[error] 12-12: This default parameter should follow the last required parameter or should be a required parameter. (lint/style/useDefaultParameterLast)

The last required parameter is here:

A default parameter that precedes a required parameter cannot be omitted at call site.
Unsafe fix: Turn the parameter into a required parameter.

Additional comments not posted (23)
app/client/src/layoutSystems/anvil/viewer/canvas/AnvilDetachedWidgets.tsx (1)

17-17: The simplification of defaultWidgetProps is noted. Ensure this does not adversely affect any widget functionality.

Verification successful

The simplification of defaultWidgetProps to an empty object is noted across multiple files. To ensure this change does not adversely affect widget functionality, it is essential to verify how defaultWidgetProps is used throughout the codebase, particularly in functions or methods that depend on these properties.

  • app/client/src/layoutSystems/common/utils/canvasUtils.ts
  • app/client/src/layoutSystems/autolayout/canvas/AutoLayoutCanvasView.tsx
  • app/client/src/layoutSystems/fixedlayout/canvas/FixedLayoutEditorCanvas.tsx
  • app/client/src/layoutSystems/fixedlayout/canvas/FixedLayoutViewerCanvas.tsx
  • app/client/src/layoutSystems/anvil/viewer/canvas/AnvilDetachedWidgets.tsx
  • app/client/src/layoutSystems/anvil/layoutComponents/WidgetRenderer.tsx

These files reference defaultWidgetProps, and it is crucial to check their context and usage.

To proceed, we need to gather more detailed information on how defaultWidgetProps is being utilized in the identified files. This will help determine if the simplification to an empty object impacts any functionality.


The detailed context from the identified files reveals that defaultWidgetProps is used in various widget rendering functions and components. The simplification of defaultWidgetProps to an empty object in AnvilDetachedWidgets.tsx and other related files appears to be consistent.

Key observations:

  • Functions like renderChildWidget and renderChildren: These functions merge defaultWidgetProps with other properties like childWidgetData and layoutSystemProps. Simplifying defaultWidgetProps to an empty object should not break functionality as long as other properties are correctly provided.
  • Components in fixedlayout, autolayout, and anvil directories: These components initialize defaultWidgetProps with specific properties relevant to their layout system. The change in AnvilDetachedWidgets.tsx aligns with this pattern.

Since no specific issues or errors related to defaultWidgetProps were identified in the search results, it appears that the simplification is safe and does not negatively impact widget functionality.

Conclusion

The simplification of defaultWidgetProps to an empty object is consistent across the codebase and should not adversely affect widget functionality.

  • app/client/src/layoutSystems/common/utils/canvasUtils.ts
  • app/client/src/layoutSystems/autolayout/canvas/AutoLayoutCanvasView.tsx
  • app/client/src/layoutSystems/fixedlayout/canvas/FixedLayoutEditorCanvas.tsx
  • app/client/src/layoutSystems/fixedlayout/canvas/FixedLayoutViewerCanvas.tsx
  • app/client/src/layoutSystems/anvil/viewer/canvas/AnvilDetachedWidgets.tsx
  • app/client/src/layoutSystems/anvil/layoutComponents/WidgetRenderer.tsx
Scripts executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Description: Verify that the simplification of defaultWidgetProps does not impact widget functionality.

# Test: Search for any issues reported or functionality changes related to widgets.
rg --type typescript $'defaultWidgetProps'

Length of output: 76



Script:

#!/bin/bash
# Description: Verify that the simplification of defaultWidgetProps does not impact widget functionality.

# Test: Search for any issues reported or functionality changes related to widgets.
rg --type ts 'defaultWidgetProps'

Length of output: 1891



Script:

#!/bin/bash
# Description: Gather detailed usage of defaultWidgetProps in the identified files.

# Extract functions or methods that utilize defaultWidgetProps in the identified files.
for file in $(rg --files-with-matches 'defaultWidgetProps' | grep -E 'app/client/src/layoutSystems/(common/utils/canvasUtils.ts|autolayout/canvas/AutoLayoutCanvasView.tsx|fixedlayout/canvas/FixedLayoutEditorCanvas.tsx|fixedlayout/canvas/FixedLayoutViewerCanvas.tsx|anvil/viewer/canvas/AnvilDetachedWidgets.tsx|anvil/layoutComponents/WidgetRenderer.tsx)'); do
  echo "File: $file"
  rg --context 5 'defaultWidgetProps' $file
done

Length of output: 8873

app/client/cypress/support/Pages/Anvil/AnvilLayout.ts (1)

13-15: The addition of verifyAnvilModalIsClosed method is a good use of existing functionality to check for the non-existence of a modal, effectively verifying its closed state.

app/client/src/layoutSystems/anvil/editor/canvasArenas/DetachedWidgetsDropArena.tsx (1)

49-52: Adding data-testid="t--anvil-detached-widgets-drop-arena" enhances the testability of the component, facilitating easier and more reliable automated testing.

app/client/cypress/support/Pages/Anvil/Locators/index.ts (2)

12-24: Added new selectors for modal elements to facilitate testing. Ensure these selectors are unique and correctly target the intended elements.


45-46: Introduced a new selector for detached widgets drop arena. Verify that this selector is used consistently across all related test scripts.

app/client/src/layoutSystems/anvil/common/AnvilFlexComponent.tsx (2)

48-48: Added widgetName prop to enhance the component's flexibility and testing capabilities.


103-111: Introduced testDataAttributes to facilitate testing. Ensure that these attributes are used consistently and are helpful in identifying UI components in tests.

app/client/cypress/support/Pages/Anvil/AnvilDnDHelper.ts (2)

Line range hint 30-74: Updated drag-and-drop logic to handle the new dropModal property. Ensure that this logic does not interfere with existing drag-and-drop functionality and that it enhances the modal handling as intended.


14-14: Added a new optional property dropModal to DropTargetDetails. Validate that this new property is handled correctly in all relevant drag-and-drop scenarios.

Verification successful

The search results indicate that the dropModal property is indeed used in several places within the codebase:

  • Definition: app/client/cypress/support/Pages/Anvil/AnvilDnDHelper.ts line 14.
  • Usage:
    • app/client/cypress/support/Pages/Anvil/AnvilDnDHelper.ts lines 14, 15, and 16.
    • app/client/cypress/e2e/Regression/ClientSide/Anvil/AnvilModal_spec.ts lines 12 and 13.

These usages suggest that dropModal is being checked and set appropriately in the drag-and-drop logic and in test scenarios.

  • app/client/cypress/support/Pages/Anvil/AnvilDnDHelper.ts:

    • Line 14: Definition of dropModal.
    • Line 15: Conditional check using dropModal.
    • Line 16: Another conditional check using dropModal.
  • app/client/cypress/e2e/Regression/ClientSide/Anvil/AnvilModal_spec.ts:

    • Lines 12 and 13: dropModal is set to true in test cases.

The presence of these checks and assignments indicates that the new dropModal property is being handled correctly in the relevant drag-and-drop scenarios.

Scripts executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Description: Verify the handling of the `dropModal` property in drag-and-drop scenarios.

# Test: Search for the usage of `dropModal`. Expect: Correct handling in all relevant scenarios.
rg --type typescript $'dropModal'

Length of output: 67



Script:

#!/bin/bash
# Step 1: Locate the definition of `DropTargetDetails`.
ast-grep --lang typescript --pattern 'interface DropTargetDetails { $$$ }'

# Step 2: Search for the usage of `dropModal` across the codebase.
rg 'dropModal'

Length of output: 963

app/client/src/widgets/wds/WDSModalWidget/widget/index.tsx (2)

Line range hint 126-150: Refactored the method getModalClassNames to dynamically generate class names based on widget properties. Ensure that this dynamic generation does not introduce any styling conflicts or errors in modal rendering.


27-28: Added imports for getAnvilWidgetDOMId and widgetTypeClassname. Verify that these utilities are used appropriately within this file and do not introduce any dependencies that could affect the widget's performance.

Verification successful

The shell script has successfully identified the usage of getAnvilWidgetDOMId and widgetTypeClassname in the specified file app/client/src/widgets/wds/WDSModalWidget/widget/index.tsx. Here are the relevant lines:

  • getAnvilWidgetDOMId is used in:

    return `${getAnvilWidgetDOMId(widgetId)} ${widgetTypeClassname(
  • widgetTypeClassname is used in:

    return `${getAnvilWidgetDOMId(widgetId)} ${widgetTypeClassname(

Both utilities are used together in a return statement that constructs a string. This suggests that they are used to generate a DOM ID and a class name for a widget. This usage appears to be appropriate and should not introduce significant performance issues.

  • app/client/src/widgets/wds/WDSModalWidget/widget/index.tsx
Scripts executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Description: Verify the appropriate use of imported utilities.

# Test: Search for the usage of `getAnvilWidgetDOMId` and `widgetTypeClassname`. Expect: Appropriate and performance-efficient usage.
rg --type typescript $'getAnvilWidgetDOMId|widgetTypeClassname'

Length of output: 99



Script:

#!/bin/bash
# Description: Verify the appropriate use of imported utilities.

# Test: Search for the usage of `getAnvilWidgetDOMId` and `widgetTypeClassname`. Expect: Appropriate and performance-efficient usage.
rg --type ts 'getAnvilWidgetDOMId|widgetTypeClassname'

Length of output: 5466

app/client/cypress/e2e/Regression/ClientSide/Anvil/AnvilModal_spec.ts (12)

1-13: Looks good! Proper structuring and importing of necessary modules and constants.


15-173: Comprehensive test suite covering various modal interactions. Let's ensure each test case is robust and well-documented.


21-23: Ensure feature flags are properly set up for Anvil tests. This is crucial for enabling specific features during testing.


25-27: Good use of setup steps to clean up the canvas before each test. This prevents state leakage between tests.


28-57: This test case verifies the opening of a modal by interacting with a button. Ensure that modal interactions are simulated accurately.


59-66: Test case for closing modal using the close icon is well-implemented. Verifies both the action and the expected outcome.


67-85: The test for closing a modal by clicking outside the modal area is crucial for user experience. Good coverage of edge cases.


87-99: ESC key functionality is a common requirement for modals. This test ensures that the modal closes as expected when the ESC key is pressed.


100-111: Testing the onClose function is essential to ensure that modals perform cleanup or other actions correctly on closure.


112-123: Similarly, verifying the onSubmit function ensures that modals handle submissions correctly. This is key for forms and data entry modals.


124-141: Drag-and-drop functionality within a modal is a complex interaction that often leads to bugs. This test is crucial for ensuring robust DnD capabilities.


143-171: Testing different modal sizes is important for responsive design. This test ensures that the modal behaves correctly across various sizes.

@@ -39,6 +54,7 @@ const anvilWidgetsLocators = {
WDSINPUT: "wdsinputwidget",
WDSSWITCH: "wdsswitchwidget",
WDSCHECKBOX: "wdscheckboxwidget",
WDSMODAL: "wdsmodalwidget",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tip

Codebase Verification

The search results indicate that the new widget type WDSMODAL is only defined in index.ts and referenced in test files, but there are no indications of its handling in the main widget system logic.

  • app/client/cypress/support/Pages/Anvil/Locators/index.ts: Definition of WDSMODAL.
  • No other occurrences in the main codebase excluding test files.

This suggests that the new widget type WDSMODAL might not be integrated into the widget system logic yet.

Analysis chain

Added a new widget type for modals. Confirm that this widget type is integrated into the widget system and handled appropriately in widget-related logic.


Let's address the issue with the rg command by specifying a recognized file type and ensuring we capture all relevant instances of WDSMODAL.


The search results show that the new widget type WDSMODAL is defined in index.ts and referenced in the AnvilModal_spec.ts test files. However, to confirm proper integration and handling in the widget system logic, we need to verify its usage beyond just test files.

Scripts executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Description: Verify the integration of the new modal widget type.

# Test: Search for the handling of the new widget type. Expect: Proper handling in the widget system logic.
rg --type typescript $'WDSMODAL'

Length of output: 66



Script:

#!/bin/bash
# Description: Verify the integration of the new modal widget type.

# Correcting the file type and searching for the handling of the new widget type in TypeScript files.
rg --type-add 'ts:*.ts' --type ts 'WDSMODAL'

Length of output: 335



Script:

#!/bin/bash
# Description: Verify the integration of the new modal widget type in the widget system logic.

# Searching for the handling of the new widget type in the entire codebase, excluding test files.
rg --type-add 'ts:*.ts' --type ts 'WDSMODAL' --glob '!**/*spec.ts'

Length of output: 153

() => {
before(() => {
// intercept features call for Anvil + WDS tests
featureFlagIntercept({
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need this? Using the ${ANVIL_EDITOR_TEST} in the test name should make the feature flagging override work right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it was added here to make sure the intercept works even after page load, will confirm this.

The one you are mentioning worked only on load of the applications page.

once we remove checks in getLayoutSystemType for anvil feature flag enabling, we can remove this block of code.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually didn't use it in my button snapshots PR and it works fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks, let me try with my tests, will remove if not needed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you use command line to generate these snapshot or used the cytest command?

Copy link
Contributor

@jsartisan jsartisan Jun 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason I ask is if you have used the cytest command, It will always pass the CI.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

matchImageSnapshot creates the snapshot the first time in the directory provided if there is no snapshot already present, once the snap is present it will do a diff against the existing snapshot.

so when I first ran the test, it created the snapshot and for the next runs it does diff checks against the already generated snapshots.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean how did you generate the snapshots? with headless chrome or regular chrome with cypress? I did with

npx cypress run --spec cypress/e2e/Regression/ClientSide/Anvil/Widgets/AnvilButtonWidgetSnapshot_spec.ts --browser chrome

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I generated via chromium based electron.

const { disableWidgetInteraction, type, widgetId, widgetName } = this.props;
return `${getAnvilWidgetDOMId(widgetId)} ${widgetTypeClassname(
type,
)} t--widget-${widgetName?.toLowerCase()} ${
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@marks0351 Didn't we decide not to use classnames for tests?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I saw this earlier as well.

@marks0351 marks0351 requested a review from a team as a code owner June 21, 2024 09:21
@marks0351 marks0351 added ok-to-test Required label for CI and removed ok-to-test Required label for CI labels Jun 21, 2024
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between ba6956e and 36727bc.

Files selected for processing (9)
  • app/client/cypress/e2e/Regression/ClientSide/Anvil/AnvilModal_spec.ts (1 hunks)
  • app/client/cypress/support/Pages/Anvil/AnvilDnDHelper.ts (3 hunks)
  • app/client/cypress/support/Pages/Anvil/Locators/index.ts (3 hunks)
  • app/client/packages/design-system/widgets/src/components/Modal/src/Modal.tsx (2 hunks)
  • app/client/packages/design-system/widgets/src/components/Modal/src/types.ts (2 hunks)
  • app/client/packages/design-system/widgets/src/components/Modal/stories/ModalExamples.tsx (2 hunks)
  • app/client/packages/design-system/widgets/src/testing/ComplexForm.tsx (1 hunks)
  • app/client/src/widgets/anvil/constants.ts (1 hunks)
  • app/client/src/widgets/wds/WDSModalWidget/widget/index.tsx (2 hunks)
Files skipped from review due to trivial changes (1)
  • app/client/src/widgets/anvil/constants.ts
Files skipped from review as they are similar to previous changes (4)
  • app/client/cypress/e2e/Regression/ClientSide/Anvil/AnvilModal_spec.ts
  • app/client/cypress/support/Pages/Anvil/AnvilDnDHelper.ts
  • app/client/cypress/support/Pages/Anvil/Locators/index.ts
  • app/client/src/widgets/wds/WDSModalWidget/widget/index.tsx
Additional comments not posted (6)
app/client/packages/design-system/widgets/src/components/Modal/src/Modal.tsx (2)

10-10: Ensure dataAttributes default value initialization.

The dataAttributes prop is correctly initialized with an empty object as a default value. This is a good practice as it ensures the component behaves predictably when the prop is not provided by the caller.


20-20: Proper propagation of dataAttributes.

The dataAttributes are properly spread into the PopoverModalContent component. This ensures that any data attributes passed to the Modal are correctly applied to the modal content, which can be crucial for accessibility and testing.

app/client/packages/design-system/widgets/src/components/Modal/src/types.ts (1)

18-18: Addition of dataAttributes to ModalProps.

The inclusion of dataAttributes in ModalProps with a type of Record<string, string> is appropriate. This allows for flexible and dynamic attribute assignment which can be utilized for testing or other DOM-specific manipulations.

app/client/packages/design-system/widgets/src/testing/ComplexForm.tsx (1)

140-140: Proper usage of dataAttributes in Modal component.

The dataAttributes prop is used to set data-size to "small". This change aligns with the global changes in the modal component's handling of size through data attributes instead of a direct prop. This can improve the flexibility and maintainability of the component's style handling.

app/client/packages/design-system/widgets/src/components/Modal/stories/ModalExamples.tsx (2)

54-54: Consistent application of dataAttributes for large size.

The dataAttributes prop is correctly used to specify the size as "large". This is consistent with the new approach of handling modal sizes through data attributes, which was observed in other files as well.


124-124: Consistent application of dataAttributes for small size.

The application of dataAttributes for setting the size to "small" is consistent and correct. This maintains uniformity across different examples and ensures that the new method of specifying modal sizes is applied throughout the codebase.
[APROVED]

@marks0351 marks0351 added ok-to-test Required label for CI and removed ok-to-test Required label for CI labels Jun 24, 2024
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 36727bc and cc291be.

Files selected for processing (1)
  • app/client/packages/design-system/widgets/src/testing/ComplexForm.tsx (1 hunks)
Files skipped from review as they are similar to previous changes (1)
  • app/client/packages/design-system/widgets/src/testing/ComplexForm.tsx

riodeuno
riodeuno previously approved these changes Jun 24, 2024
@marks0351 marks0351 removed the ok-to-test Required label for CI label Jun 24, 2024
@marks0351 marks0351 added the ok-to-test Required label for CI label Jun 24, 2024
@marks0351 marks0351 added ok-to-test Required label for CI and removed ok-to-test Required label for CI labels Jun 24, 2024
@marks0351
Copy link
Contributor Author

/build-deploy-preview skip-tests=true

Copy link

Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/9641213540.
Workflow: On demand build Docker image and deploy preview.
skip-tests: true.
env: ``.
PR: 34347.
recreate: .

Copy link

Deploy-Preview-URL: https://ce-34347.dp.appsmith.com

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between cc291be and 75d8ffb.

Files ignored due to path filters (3)
  • app/client/cypress/snapshots/AnvilModal_spec.ts/anvil/anvilModalLargeSize.snap.png is excluded by !**/*.png
  • app/client/cypress/snapshots/AnvilModal_spec.ts/anvil/anvilModalMediumSize.snap.png is excluded by !**/*.png
  • app/client/cypress/snapshots/AnvilModal_spec.ts/anvil/anvilModalSmallSize.snap.png is excluded by !**/*.png
Files selected for processing (1)
  • app/client/src/pages/Editor/Explorer/EntityExplorer.tsx (1 hunks)
Files skipped from review due to trivial changes (1)
  • app/client/src/pages/Editor/Explorer/EntityExplorer.tsx

@marks0351 marks0351 added ok-to-test Required label for CI and removed ok-to-test Required label for CI labels Jun 24, 2024
@jsartisan jsartisan added ok-to-test Required label for CI and removed ok-to-test Required label for CI labels Jun 24, 2024
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 75d8ffb and bcc4bbd.

Files ignored due to path filters (3)
  • app/client/cypress/snapshots/AnvilModal_spec.ts/anvilModalLargeSize.snap.png is excluded by !**/*.png
  • app/client/cypress/snapshots/AnvilModal_spec.ts/anvilModalMediumSize.snap.png is excluded by !**/*.png
  • app/client/cypress/snapshots/AnvilModal_spec.ts/anvilModalSmallSize.snap.png is excluded by !**/*.png
Files selected for processing (1)
  • app/client/cypress/e2e/Regression/ClientSide/Anvil/AnvilModal_spec.ts (1 hunks)
Files skipped from review as they are similar to previous changes (1)
  • app/client/cypress/e2e/Regression/ClientSide/Anvil/AnvilModal_spec.ts

@marks0351 marks0351 merged commit ed497db into release Jun 24, 2024
50 checks passed
@marks0351 marks0351 deleted the anvil-tests branch June 24, 2024 10:58
Shivam-z pushed a commit to Shivam-z/appsmith that referenced this pull request Jul 10, 2024
[![workerB](https://img.shields.io/endpoint?url=https%3A%2F%2Fworkerb.linearb.io%2Fv2%2Fbadge%2Fprivate%2FU2FsdGVkX1n8wGoml2WVG4C2fL4hX6Z1rh6k4aeSvE%2Fcollaboration.svg%3FcacheSeconds%3D60)](https://workerb.linearb.io/v2/badge/collaboration-page?magicLinkId=9GEnyEC)
## Description
> [!TIP]  
> _Add a TL;DR when the description is longer than 500 words or
extremely technical (helps the content, marketing, and DevRel team)._
>
> _Please also include relevant motivation and context. List any
dependencies that are required for this change. Add links to Notion,
Figma or any other documents that might be relevant to the PR._


Fixes appsmithorg#33740 
_or_  
Fixes `Issue URL`
> [!WARNING]  
> _If no issue exists, please create an issue first, and check with the
maintainers if the issue is valid._

## Automation

/ok-to-test tags="@tag.Anvil"

### 🔍 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/9643638292>
> Commit: bcc4bbd
> <a
href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=9643638292&attempt=1"
target="_blank">Cypress dashboard</a>.
> Tags: `@tag.Anvil`

<!-- end of auto-generated comment: Cypress test results  -->









## Communication
Should the DevRel and Marketing teams inform users about this change?
- [ ] Yes
- [ ] No


<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

- **New Features**
- Introduced test cases for Anvil modals, covering interactions like
opening, closing, drag and drop operations, and handling modal
functions.

- **Bug Fixes**
- Enhanced testing capabilities with `data-testid` attributes for better
identification and testing of components, particularly in detached
widget drop areas.

- **Refactor**
- Updated the `Modal` component to use `dataAttributes` instead of
`size` prop to streamline attribute handling.
- Dynamic generation of modal class names based on properties for better
styling and consistency.

- **Style**
- Adjusted styling for SVG elements within the `EntityExplorer`
component, specifically modifying the height and width properties.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->

---------

Co-authored-by: Pawan Kumar <pawankumar@Pawans-MacBook-Pro-2.local>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Anvil Pod Issue related to Anvil project ok-to-test Required label for CI potential-duplicate This label marks issues that are potential duplicates of already open issues skip-changelog Adding this label to a PR prevents it from being listed in the changelog Task A simple Todo
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Task]: Test cases for Anvil Modal Widget
3 participants