appsmith icon indicating copy to clipboard operation
appsmith copied to clipboard

[Epic] Dynamically Change height for widgets (New Size Options for Container and other widgets)

Open momcilo-appsmith opened this issue 3 years ago • 31 comments

Objective

Enable widgets to mimic standard HTML block elements by automatically extending the height to fit contents.

Success Metrics *

Goal Metric
Mimic HTML block component behaviour Feature enabled numbers in deployed apps? (TBD)

Requirements

Requirement User story
Block or "Container" like widgets extend height to fit all contents Web developers are used to seeing block elements extend until configured with a fixed height (Standard HTML block element behaviour). Appsmith developers can now find a parallel behaviour in Appsmith
Elements have a concept of max-height, a configurable property One of the common CSS properties used by web developers to restrict how much an element can "grow" in size, is using the max-height property. Appsmith developers can now mimic this in Appsmith
Hide this feature from certain widgets, that do not allow this behaviour Not all HTML elements increase in height based on contents, especially the ones which don't have contents like br and hr, similarly, widget developers can configure if a widget can extend height dynamically in widget configurations

DX updates

  • Widgets will have a new property called Height with two options in a dropdown FIXED, HUG_CONTENTS

  • When the property above is HUG_CONTENTS, Appsmith developers will see more properties called Max Height in Rows : number and Min Height in Rows: number. These define the maximum/minimum height to which the widget can grow/shrink automatically.

  • Default Min Height is 2 rows (this is also the minimum value which can be configured), while Default Max Height is 1000 rows or some arbitrarily large value.

  • FIXED: This is the current mode for widgets, where the heights once defined, shall not be changed

  • HUG_CONTENTS:

    • In the edit mode, the height will change dynamically, and Appsmith developers will not be allowed to manually resize the height. These changes will be persisted in the DSL.
    • In the view mode, the height will change dynamically but the values will not be persisted in the DSL.

Note: These properties do not conflict with "Scroll contents" property as once the height is dynamic, the widget will not have a scroll

Documentation

  • A detailed documentation with screen recordings demonstrating the logic of reflow
  • A detailed documentation with screen recordings demonstrating the two new properties

Reflow Logic (UX)

  • ~~Reflow logic will follow the current drag and drop reflow logic for consistency.~~ This has been changed to have relative reflow (See comment)
  • ~~If Appsmith developers need to maintain relative positions of widgets, the widgets can be grouped into a transparent container.~~

Example demonstration of before and after:

  • New Images TBD

Developer Handoff Document in Figma

https://www.figma.com/file/Yk4V1PQFbL9Cmti1lemqFd/?node-id=442%3A90845

RACI Matrix

Role Members
Responsible @riodeuno @ankurrsinghal
Accountable @riodeuno
Consulted @momcilo-appsmith @areyabhishek @Nikhil-Nandagopal @rahulramesha @vinay-appsmith
Informed @somangshu @vuiets @rohitagarwal88

Front logo Front conversations

momcilo-appsmith avatar Feb 09 '22 09:02 momcilo-appsmith

@momcilo-appsmith i understood the concept, but did not understand how this can be built. Keeping dynamic height aside for a min.

Main area of confusion was how will we align 2 items to be in the same line/row. Here it automatically splits the container into multiple rows and only adds a new row in hug contents. Does it mean we need to start thinking in terms of rows in containers?

dilippitchika avatar Feb 10 '22 14:02 dilippitchika

@dilippitchika We don't need to align them, that is just how it had to be shown in Figma.

momcilo-appsmith avatar Feb 13 '22 12:02 momcilo-appsmith

From #4024 we also wanted to allow dynamic helper text that is below the input component and needs additional height, It will have to reflow other widgets. Our alternate solution before this was to introduce a tooltip for helper.

somangshu avatar Feb 16 '22 08:02 somangshu

Another conversation on the same topic in a private thread.

@areyabhishek : We seriously need to fix our text widget. The weird vertical way we cut the top and bottom of the widget when there's no space and the way it introduces the scroll is baad. Every app looks terrible on my screen in edit mode because the text is always chopped off. Screen Shot 2022-02-13 at 9 38 17 PM

somangshu avatar Feb 16 '22 08:02 somangshu

@momcilo-appsmith thank you for writing this it looks great! I have a few points here

  • Scroll and this property do seem to be related and if a user enables dynamic height, then scroll should be hidden
  • Instead of having number of rows as a max height which a user has to guess, why not simply have a property "Enable Max Height" which sets the current height as the max height? This is easier for a user to configure what the correct max height should be.

Nikhil-Nandagopal avatar Feb 18 '22 10:02 Nikhil-Nandagopal

@Nikhil-Nandagopal

  • We can extend height to a limit, after which the developer needs to decide whether the content is clipped (today's default), or content is scrolled. This is why I believe these are exclusive. How do you imagine this feature working in conjunction with scroll?

  • Regarding the max height -- do you mean, something like "Use current height as max height"?

riodeuno avatar Feb 18 '22 11:02 riodeuno

@riodeuno I think my point is scroll will only come if there is a max height set otherwise it won't Yes just like that "Use current height as max height". This makes it easier to configure than guessing what the UI might look like and what the correct max height value should be

Nikhil-Nandagopal avatar Feb 18 '22 11:02 Nikhil-Nandagopal

I think my point is scroll will only come if there is a max height set otherwise it won't

Agreed!

This makes it easier to configure than guessing what the UI might look like and what the correct max height value should be

Can you elaborate the process? I am trying to imagine the DX as follows

  • Developer enables dynamic height
  • Developer resizes widget to increase height
  • Developer enables "use current height as max height"
  • Developer resizes widget back to decrease height

One alternative I can think of, regarding the numerical row input is to have a marker which marks a widget which has a dynamic height. dynamic-height

riodeuno avatar Feb 18 '22 12:02 riodeuno

@riodeuno got the problem thanks for clarifying. My suggestion DX doesn't make sense then :) Can we think of something better for this? Guessing the correct max width / height has always been one of my struggles as a developer. The UI always goes bonkers when I can't visualize what the worst case can look like

Nikhil-Nandagopal avatar Feb 18 '22 12:02 Nikhil-Nandagopal

@Nikhil-Nandagopal Sounds good. I can relate to the inaccuracy in anticipating how a feature effects things at runtime. 😅

@momcilo-appsmith Any suggestions on a good UX here?

riodeuno avatar Feb 18 '22 13:02 riodeuno

Adding screenshots to demonstrate the experience we want to create image image image

momcilo-appsmith avatar Feb 22 '22 08:02 momcilo-appsmith

Update

  • Why are we not using dynamic width?

    • Appsmith doesn't have a concept of infinitely wide canvas. There is always a fixed maximum number of columns a widget can occupy.
    • In web design, horizontal scrolls are usually not a good idea, as mouse users cannot easily navigate this without buttons.
    • If we really do need dynamic width, we'll look into it as a subsequent effort.
  • Why max-height?

    • This constraint is what web developers have come to expect when something changes dimensions in an uncontrolled manner.
    • It is better to have this option, as a feasibility improvement, than not having it.
  • Will this work in edit mode for container like widgets?

    • Yes. Which means that if we drag or resize a child of the container widget, the container widget should increase height to adapt to this, if needed.
  • Appsmith developers can be surprised with the changes in layout in the view mode!!

    • Causes for surprises (identified so far):

      • No idea what 100 rows of max-height means.
      • No idea what widgets might get displaced when a widget extends height
    • Solutions:

      • UX to indicate possible changes to a widget's height, without making the UI too busy
      • Relative reflow. Maintains padding with the "pushed" widgets, and within extending containers, as that's how we expect it to work in a web application
  • How will this handle the invisible widgets?

    • It will consider them as existing widgets in edit mode
    • It will consider them non-existent widgets in view mode.
    • Special handling is out of scope for this project
    • We will try to accommodate discoverability when dealing with the UX mentioned in the previous point.

cc: @Nikhil-Nandagopal @momcilo-appsmith @areyabhishek

riodeuno avatar Feb 25 '22 09:02 riodeuno

@dilippitchika @riodeuno we also need to define which widgets will be dynamic by default vs which will need a user opt-in. Because for example if table is dynamic by default, it will crash the app. Similarly, dynamic height in the chart, audio widget, iframe doesn't make sense. We need to think carefully how this will affect every single widget

Nikhil-Nandagopal avatar Feb 25 '22 11:02 Nikhil-Nandagopal

@Nikhil-Nandagopal Absolutely. I'll make sure to add this to the task list for the project.

riodeuno avatar Feb 25 '22 12:02 riodeuno

Action Items

Upcoming week

  • [ ] Feature Implementation design @riodeuno @ankurrsinghal
    • [ ] Review of code design with emphasis on performance with @SatishGandham, @mohanarpit and @rahulramesha
  • [x] Identify all widgets that needs this (Parallel task) @somangshu @dilippitchika @riodeuno

Spinoff project ( This will run in parallel to implementation related tasks )

  • [x] Design solution for Appsmith developers can be surprised with the changes in layout in the view mode!! @momcilo-appsmith

cc @rohitagarwal88

somangshu avatar Mar 04 '22 08:03 somangshu

@riodeuno @ankurrsinghal Here is the Handoff file: https://www.figma.com/file/VtsF6ygzHI6gmsOZC3rGnK/Dynamic-Height-Mon-14-March-2022?node-id=2%3A1 Please take a look and let me know if you have any questions or feedback.

momcilo-appsmith avatar Mar 14 '22 08:03 momcilo-appsmith

There's something which I wanted to raise for table and list widget, we have a pagesize which is calculated for each table and list. How will the max height property be used if the data is coming according to the page size which is currently set?

The only scenario i found this useful was to prevent scroll on word-wrap. @somangshu @riodeuno

CC - @sbalaji1192

dilippitchika avatar Mar 14 '22 08:03 dilippitchika

@dilippitchika This is a great point! I believe, if we can confirm that the dynamic height will come into play only if the word-wrap is enabled, then we can make sure to make the dynamic height property available accordingly.

riodeuno avatar Mar 21 '22 08:03 riodeuno

Dynamic height investigation results

Approach

  • We assumed using ref to be the most straightforward approach to enable dynamic height in widgets.
  • This works by attaching refs to all widget components, that are forwarded from the Widget in concern. The refs are creatd in the BaseWidget abstract to make sure they're expected to be avaialable in all widgets whether they decide to enable dynamic height or not.
  • We identified the widgets within our current set of widgets -- that would have an impact to Appsmith application development -- that will have the dynamic height option.
  • We looked into how to forward refs within the list of widgets identified.
  • We use the scrollHeight property of the component's wrapping element to determine if the widget's height needs to be adjusted. This means, that all components should cover all of the widget's area.

General findings

  • All widgets will have a new prop passed down to their respective components -- ref -- which is defined in the BaseWidget (React.createRef), hence can be accessed using this.contentRef in any of our widgets.

  • All widget components will now need to have to be wrapped into the React.forwardRef HOC to safely propagate these refs.

  • Some widget components utilise refs internally, if these refs are added to the top level wrapping element in the component, then they will need to re-use the ref passed down from the widget, instead of creating a ref within the component.

  • All widget components need to be wrapped in an HTMLElement (via styled-components or otherwise). This means that usage of custom components from blueprintjs or other libraries which donot forward the ref prop safely to their underlying HTML element is forbidden. For example, we can't use the Text component provided by blueprintjs as the top level wrapper in the TextWidget anymore. * Note: This is still under investigation, if we can find a way to manage this, this requirement may be removed *

  • Based on the above observations, we need to be also making changes to the widget components in case they violate any of the rules above.

  • We will be propagating the dynamic heights via a redux state instead of computing it on the fly in the Base Widget for the following reasons

    • Changes in one widget may cause changes in the ancestors as well
    • Changes in one widget need to reflow other widgets based on the empty space created or new space occupied
    • To avoid re-renders and make sure computations only update state when necessary.

Collateral fixes

  • Empty space at the bottom in the Main Container in view mode can be removed with this solution
  • Empty space created dynamically (hiding widgets, or reducing their height) can potentially be filled by reflowing other widgets
  • A more detailed template for widget components will be created. Not really a fix.

Concerns

  • Standardisation: We need a way to enforce the standards for component code, so that widget developers cannot fail to have the dynamic height work correctly for the widgets they implement.
  • Filling empty space created when a widget is hidden, is still under product discussions and will need more time to reach a conclusion, leading to a potential code change in later stages of the project.

Canvas Widget

  • This widget has a few quirks where it differs in how it renders on the edit mode and the view mode. We've looked into it to a certain extent, not all quirks have been tackled, however, we are confident that this will not be a blocker for the project.

  • A few points of note

    • CanvasWidget has more rows than its parent if it canExtend, i.e, if the parent can scroll contents.
    • CanvasWidget renders the DropTarget in the edit mode and renders a simple div in view mode.
    • CanvasWidget already has dynamic height, in all scenarios, it is the parent which needs to adapt to the changes in the height of the Canvas Widget based on the Canvas Widget's contents. This is another quirk, because an update to the parent's height, causes an update in the CanvasWidget's height, which if the dynamic height feature doesn't handle, will cause the parent's height to increase again, leading to an infinite loop.

Container Widget

  • Already has a block div element as its child, which is the Canvas Widget. Changes in the Canvas Widget's contents will cause the Container Widget to change in height.
  • Refs are propagated Canvas Widget whose scrollHeight is the property to decided any changes.
  • Refs were being used in the container component which now need to move to the context ref

Form Widget

  • Same as the Container Widget

Tabs Widget

  • Same as the Container Widget

List Widget

  • Needs product update, we haven't investigated this yet.

Switch Group Widget

  • Using the React.forwardRef, we can set the base widget contentRef to the SwitchGroupContainer(which is a normal styled div) ref and as soon as we have more options the base widget will grow dynamically.

Checkbox Group Widget

  • Same as Switch Group Widget

Radio Group Widget

  • Same as Switch Group Widget but here is one issue, the Radio group container is a styled RadioGroup component from blueprint so we can't access the internal ref to the div. To fix this we can wrap the container in another simple plain div and forward the ref to its ref.

Rating Widget

  • Same as Switch Group Widget but here the issue is it does not updates on mount but on update.

Table Widget

  • Table widget needs Dynamic Height when we enable Server Side Pagination, where we forward the ref to table-body container.
  • One issue here is that, reflow wrapper is setting some computed height which inhibits the table container to grow as much as the base widget height which updates because of ref's scrollHeight.

Input Widget

  • Input widget problem arises when we have a very long label it overflows the input field to fit the container.
  • One thing we can do using Dynamic Height is that as soon as base widget updates its height using scrollHeight, the label and input field becomes block level which looks better.

Checkbox Widget

  • Same as input widget.

Switch Widget

  • Same as input widget.

Input Currency Widget

  • Same as input widget.

Input Phone Widget

  • Same as input widget.

Select Widget

  • Same as input widget.

Multi-select Widget

  • Same as input widget.

Tree select Widget

  • Same as input widget.

Multi-select Tree Widget

  • Same as input widget.

Datepicker Widget

  • Same as input widget (Currently, Datepicker does not have a label property).

JSONForm Widget

  • Same as Switch Group Widget. Here we will forward the ref to StyledFormBody's ref.

riodeuno avatar Apr 18 '22 06:04 riodeuno

Update

  • Conclusion: Disable vertical resize of the widget when dynamic height is enabled
  • Parallel design exploration: Signifiers of max-height and min-height. @vinay-appsmith @momcilo-appsmith
  • Conclusion: Persist heights in edit mode when they change dynamically.

cc @rohitagarwal88 @ankurrsinghal

riodeuno avatar Apr 21 '22 09:04 riodeuno

Update 6th May:

  • Min-Max height signifiers Design - In progress @momcilo-appsmith @vinay-appsmith
  • POC development - In Progress @ankurrsinghal @riodeuno - ETA 16th May
  • Technical hurdle: Undo-Redo does not work well with the dynamic height - Details to follow.

riodeuno avatar May 06 '22 07:05 riodeuno

Update 27th May

Tasks for this week:

  • [ ] @riodeuno Complete the dynamic height layout update algorithm
  • [ ] @ankurrsinghal Complete implementing updates to the widgets to have them respond correctly to content changes
  • [ ] @btsgh @Sripriya93 Work on compiling test scenarios and strategy for QA of the feature.
  • [ ] @rohitagarwal88 Facilitate communication with the App viewers for resolving blockers on @ankurrsinghal 's tasks

riodeuno avatar May 27 '22 09:05 riodeuno

For today and the upcoming week these are my actionable:-

  1. Resolving the conflicts of the base widget branch to fix the flaky tests in 2 pending PR's for standardization.

  2. Fixing CSS of Label Widgets:-

    1. Input, InputPhone, InputCurrency
    2. Multi-Select, Multi-Tree Select, Select, Tree Select
    3. Switch Group, Radio Group, Checkbox Group
  3. Fixing the CSS of Label of Switch, Checkbox widgets.

  4. Fixing the CSS of JSON Form Widget (if any).

  5. Fixing the CSS/JS of Table widget where it scrolls 2 rows less.

  6. Fixing the CSS of Text widget height issue.

  7. Beginning the design of th overlay for min and max height if I have bandwidth left.

ankurrsinghal avatar May 27 '22 10:05 ankurrsinghal

@ankurrsinghal @riodeuno is there a DP we can try out?

Nikhil-Nandagopal avatar May 31 '22 06:05 Nikhil-Nandagopal

@Nikhil-Nandagopal Not yet. We're completing some of the core logic related to the height updates. As well as the actionable posted by @ankurrsinghal.We should be able to share deploy preview to try this out early next week.

riodeuno avatar Jun 01 '22 06:06 riodeuno

Update 13th June

  • [x] General design frozen for the min-max signifiers @momcilo-appsmith
  • [x] To understand if percentage is a better unit than rows, we'll create a PoC for the percentages. It is confirmed that w.r.t designs there will not be any major changes in either case. @momcilo-appsmith @riodeuno
  • [x] Container update logic for widget addition, move, deletion, cut, paste, undo and redo. (This has bugs which we'll resolve before sharing the deploy preview for feedback) @rahulramesha

Targets for this week.

  • [ ] Share POC with the team, without the DX designed by @momcilo-appsmith @riodeuno @ankurrsinghal
  • [x] Remove Dynamic height project from this week's sprint backlog to avoid skewing numbers on zenhub @rohitagarwal88
  • [x] Split the DX implementation task into granular issues and associate it with the epic @riodeuno
  • [x] Overall Test strategy, QA alignment to goals and detailed checklist for test cases. @btsgh @Sripriya93

riodeuno avatar Jun 13 '22 08:06 riodeuno

@Nikhil-Nandagopal Here is an early preview of the feature. This doesn't have the visual signifiers, but has all features implemented. We're dealing with some issues, hence it is an early preview.

https://appsmith-kqx3h5521-get-appsmith.vercel.app/

riodeuno avatar Jun 24 '22 09:06 riodeuno

@Nikhil-Nandagopal @btsgh @Sripriya93 A new POC with range sliders for min max controls is available for feedback. Please check it out: https://appsmith-git-poc-dynamic-height-range-slider-get-appsmith.vercel.app/

riodeuno avatar Jul 20 '22 12:07 riodeuno

@Nikhil-Nandagopal Thank you for your reply #16783. As you said, we can consider making this a property of the table. I will add a property to TableWidget2 to let users choose whether to use fixed pageSize or dynamically calculate pageSize based on dragging or resizing.

huaningshiji avatar Sep 19 '22 07:09 huaningshiji

Hello. I was encouraged by Appsmith support (Amelia) to write a comment here, as I requested being able to make a list view with dynamic height (so all list items is visible on the page, without pagination).

I don't remember when I've last made a fixed sized window in a web app, and expected pagination to be an option rather than a requirement. It also turned hard to decide how tall to make the listview, as some phones would then show the pagination, but others would have the pagination bar outside the bottom of the screen, and thus people had to scroll anyways to reach it. Another solution was to make it small enough to fit all mobile screens, but then we have the next issue with big phones having lots of wasted space.

I see the ListView (and other views no having dynamic height) the same as if this GitHub issue would have a fixed height box, containing all the comments, which you could scroll inside. I don't think that makes sense, compared to what they actually have, where you scroll down through the page, and in the bottom you'll find the commenting section :)

I hope this feature will be part of Appsmith, as I otherwise like what Appsmith seems to offer 👍

dentych avatar Oct 17 '22 19:10 dentych