fix: Extra Space at the end of the Canvas in Viewmode #16747
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
github-actions
bot
added
Bug
| @@ -79,6 +79,7 @@ export const GridDefaults = { | |||
| DEFAULT_GRID_COLUMNS: 64, | |||
| DEFAULT_GRID_ROW_HEIGHT: 10, | |||
| CANVAS_EXTENSION_OFFSET: 2, | |||
| MAIN_CANVAS_EXTENSION_OFFSET: 8, | |||
There was a problem hiding this comment.
@rahulramesha What's the rationale? Can't we use the CANVAS_EXTENSION_OFFSET here?
There was a problem hiding this comment.
@rahulramesha I thought we were changing this to 5. Have we reconsidered?
| return Math.ceil( | ||
| Math.max(minBottomRow + GridDefaults.CANVAS_EXTENSION_OFFSET, defaultRows), | ||
| ); | ||
| const canvasOffset = |
There was a problem hiding this comment.
@rahulramesha why a different offset for main canvas, if it is to decrease annoyance then its fine but i see the offset being applied in view mode as well which creates a scroll when a page is created exactly for the view port height.
There was a problem hiding this comment.
@riodeuno @marks0351 The main rationale for MAIN_CANVAS_EXTENSION_OFFSET was that, CANVAS_EXTENSION_OFFSET is too small for mainCanvas. On edit mode, if the same CANVAS_EXTENSION_OFFSET is used, it looks as if the canvas grids abruptly ends at the last widget position and doesn't looks good while dragging. On View mode, it acts as a padding below the last widget, so that the bottom most widgets are easily accessible to interact rather than at the border of the browser, It does have a bit of scroll only if canvas goes beyond a certain length but not by much.
There was a problem hiding this comment.
why do we need this padding in view mode? did we have any feedback regarding usability? It's better if we do this only out of necessity else just skip and have the height equal to the bottom most widgets bottom row.
There was a problem hiding this comment.
@marks0351 This is the one i was mentioning, you could check the below image, with regular canvas offset (on release). It makes part of the button non interactive
Same app on this DP,
There was a problem hiding this comment.
yea it makes sense to implement this to accommodate this particular area of the canvas but in ee apps i think we have the provision to hide the appsmith label and there it might be an unwanted feature and i dont recommend having a flag to deal with that. if we are going to fix this lets fix it as a feature by giving the ability to the user to set their bottom row. what do you guys think?
There was a problem hiding this comment.
Providing the feature to set their bottom row which is intuitive to the user might require designs and UX. We can pursue that approach as well but since this works for more cases and solves the long canvas issue, I think the extra 8 rows wont make a difference since most websites have a padding at the bottom. thoughts?
|
/ok-to-test sha=27c3c42 |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/3092864776. |
|
UI Performance test run logs and artifacts: https://github.com/appsmithorg/appsmith/actions/runs/3092864776. Click to view performance test results| | Run 1 (ms)| Run 2 (ms)| Run 3 (ms)| Run 4 (ms)| Run 5 (ms)| Minimum (ms)| Median (ms)| Mean (ms)| Range (%) | SD.Sample (%) | SD.Population (%)| |
|
/ok-to-test sha=319e76c |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/3099076893. |
|
UI Performance test run logs and artifacts: https://github.com/appsmithorg/appsmith/actions/runs/3099076893. Click to view performance test results| | Run 1 (ms)| Run 2 (ms)| Run 3 (ms)| Run 4 (ms)| Run 5 (ms)| Minimum (ms)| Median (ms)| Mean (ms)| Range (%) | SD.Sample (%) | SD.Population (%)| |
rahulramesha
dismissed stale reviews from marks0351 and riodeuno
via
a27e1b2
|
/ok-to-test sha=a27e1b2 |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/3122229740. |
|
UI Performance test run logs and artifacts: https://github.com/appsmithorg/appsmith/actions/runs/3122229740. Click to view performance test results
|
| * @param finalWidgets | ||
| * @param parentId | ||
| */ | ||
| export function resizeCanvasToLowestWidget( |
There was a problem hiding this comment.
@rahulramesha I am concerned that this is a mutating function. Do you think we could return a value from this? There are two concerns I have.
- It makes testing easier, no need for cloning.
- This function only changes one value. It should be fairly simple to add a line and update these in the
finalWidgetswhere necessary.
There was a problem hiding this comment.
@riodeuno got it, I had just moved the method from another file. I agree with the points will change it to return the value instead of mutating it
| parentId === MAIN_CONTAINER_WIDGET_ID | ||
| ? GridDefaults.MAIN_CANVAS_EXTENSION_OFFSET | ||
| : GridDefaults.CANVAS_EXTENSION_OFFSET; | ||
| finalWidgets[parentId].bottomRow = Math.max( |
There was a problem hiding this comment.
Should this be Math.min instead?
There was a problem hiding this comment.
It is supposed to be Math.max, because it is comparing the maximum value of the lowest bottom row of the widget or the bottom row that is required to fill the entire height of the browser window.
|
/ok-to-test sha=2cb25ba |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/3137715113. |
|
UI Performance test run logs and artifacts: https://github.com/appsmithorg/appsmith/actions/runs/3137715113. Click to view performance test results
|
|
/ok-to-test sha=f829617 |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/3150695462. |
|
UI Performance test run logs and artifacts: https://github.com/appsmithorg/appsmith/actions/runs/3150695462. Click to view performance test results
|
Description
We had logic to fix MainCanvas height when a Widget is dragged, resized or deleted. Extending that logic to When a widget is added to mainCanvas as well.
Also added logic to trim mainCanvas width in View Mode.
Fixes #12672
Type of change
How Has This Been Tested?
Manual UI and Jest test
Checklist: