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

fix: modal deselect issue auto height #18750

Merged
merged 3 commits into from
Dec 11, 2022

Conversation

ankurrsinghal
Copy link
Contributor

Description

-- Issue
The modal widget deselects when it's changed from AutoHeight to Fixed and vice-versa.

-- Reason
When Auto Height is enabled every widget is wrapped around AutoHeightContainerWrapper in BaseWidget but for canvas-like widgets, in this case, Modal Widget, it just returns its children back, so no change in the UI as such, but for react it's a completely new node and therefore it unmounts the previous component, and ModalWidget in one of its effect, deselects itself on unmount.

-- Fix
Added another check for ModalWidget using its detachFromLayout props before wrapping AutoHeightContainerWrapper in BaseWidget class. This allows ModalWidget to never enter AutoHeightContainerWrapper ever therefore the node remains the same hence no unmount.

Fixes #18697

Media
(https://www.loom.com/share/85fe3285d14944d680c4ebfe083f601a)

Type of change

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

  • Manual

Test Plan

Add Testsmith test cases links that relate to this PR

Issues raised during DP testing

Link issues raised during DP testing for better visiblity and tracking (copy link from comments dropped on this PR)

Checklist:

Dev activity

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • PR is being merged under a feature flag

QA activity:

  • Test plan has been approved by relevant developers
  • Test plan has been peer reviewed by QA
  • Cypress test cases have been added and approved by either SDET or manual QA
  • Organized project review call with relevant stakeholders after Round 1/2 of QA
  • Added Test Plan Approved label after reveiwing all Cypress test

@vercel
Copy link

vercel bot commented Dec 7, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
appsmith ✅ Ready (Inspect) Visit Preview Dec 11, 2022 at 8:03AM (UTC)

@github-actions github-actions bot added App Viewers Pod This label assigns issues to the app viewers pod Auto Height Issues related to dynamic height of widgets Bug Something isn't working Medium Issues that frustrate users due to poor UX Modal Widget Needs Triaging Needs attention from maintainers to triage Regressed Scenarios that were working before but have now regressed UI Builders Pod Issues that UI Builders face using appsmith labels Dec 7, 2022
@ankurrsinghal
Copy link
Contributor Author

/ok-to-test sha=062f20c

@github-actions
Copy link

github-actions bot commented Dec 7, 2022

Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/3638650029.
Workflow: Appsmith External Integration Test Workflow.
Commit: 062f20c.
PR: 18750.
Perf tests will be available at https://app.appsmith.com/app/performance-infra-dashboard/pr-details-638dd7cd2913ba43778b915e?pr=18750&runId=3638650029_1

Copy link
Contributor

@riodeuno riodeuno left a comment

Choose a reason for hiding this comment

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

This is a good solution, makes things faster. We do have to make sure there aren't any side effects.

@kamakshibhat-appsmith
Copy link

kamakshibhat-appsmith commented Dec 7, 2022

Checked the fix along with other use cases w.r.t modal widgets
Looks good, we can proceed @ankurrsinghal

@rohitagarwal88 rohitagarwal88 removed the UI Builders Pod Issues that UI Builders face using appsmith label Dec 7, 2022
@github-actions github-actions bot added the UI Builders Pod Issues that UI Builders face using appsmith label Dec 7, 2022
@ankurrsinghal
Copy link
Contributor Author

/ok-to-test sha=7dad083

@github-actions
Copy link

github-actions bot commented Dec 8, 2022

Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/3648881787.
Workflow: Appsmith External Integration Test Workflow.
Commit: 7dad083.
PR: 18750.
Perf tests will be available at https://app.appsmith.com/app/performance-infra-dashboard/pr-details-638dd7cd2913ba43778b915e?pr=18750&runId=3648881787_1

@ankurrsinghal
Copy link
Contributor Author

/ok-to-test sha=812fcfb

@github-actions
Copy link

Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/3668031306.
Workflow: Appsmith External Integration Test Workflow.
Commit: 812fcfb.
PR: 18750.
Perf tests will be available at https://app.appsmith.com/app/performance-infra-dashboard/pr-details-638dd7cd2913ba43778b915e?pr=18750&runId=3668031306_1

@ankurrsinghal ankurrsinghal merged commit 432e513 into release Dec 11, 2022
@ankurrsinghal ankurrsinghal deleted the fix/modal-deselect-auto-height branch December 11, 2022 15:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
App Viewers Pod This label assigns issues to the app viewers pod Auto Height Issues related to dynamic height of widgets Bug Something isn't working Medium Issues that frustrate users due to poor UX Modal Widget Needs Triaging Needs attention from maintainers to triage Regressed Scenarios that were working before but have now regressed UI Builders Pod Issues that UI Builders face using appsmith
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: the widget gets deselected when we change height property in Modal widget
4 participants