appsmith icon indicating copy to clipboard operation
appsmith copied to clipboard

fix: resizable max height

Open riodeuno opened this issue 2 years ago • 0 comments

Description

Reported issue

In a container widget, when we switch to auto height with limits (in the property pane), then switch back to FIXED (in the property pane), we are no longer able to vertically resize the container widget to be larger.

Results of RCA

This was happening because, we set a maxDynamicHeight when we switch to auto height with limits. When we switched back to fixed, the same limits were being respected by the resizing library. This resulted in the height being capped at what was set in the maxDynamicHeight.

Code design issue:

There was also an issue where the resizing library was "aware" of the "dynamic height". Dynamic height is a platform construct and the resizing library being aware of this construct is a breach in the interface.

Solution:

We now pass the maxHeightInPx and autoHeight to the resizing library. maxHeightInPx is the maximum height -- the resizing library can -- allow in vertical resize. autoHeight ignores the resizing library's ability to set the height (in this scenario we handle the height using the dynamic height feature in the platform)

This allows us to keep the resizing library agnostic of the platform dynamic height feature and lets the platform code decide the max height possible.

PR fixes following issue(s)

Fixes #22439

Media

Type of change

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

Testing

How Has This Been Tested?

  • [x] Manual
  • [ ] Jest
  • [ ] Cypress - Limitations in the testing infra makes it difficult to add a test for this

Test Plan

Issues raised during DP testing

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:

  • [ ] Speedbreak features have been covered
  • [ ] Test plan covers all impacted features and areas of interest
  • [ ] Test plan has been peer reviewed by project stakeholders and other QA members
  • [ ] Manually tested functionality on DP
  • [ ] We had an implementation alignment call with stakeholders post QA Round 2
  • [ ] Cypress test cases have been added and approved by SDET/manual QA
  • [ ] Added Test Plan Approved label after Cypress tests were reviewed
  • [ ] Added Test Plan Approved label after JUnit tests were reviewed

riodeuno avatar May 09 '23 14:05 riodeuno