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.

Sign up for GitHub

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

Jump to bottom

fix: Canvas jitters on page refresh on an application where widgets don't occupy the entire width of the canvas #13917

Merged
merged 6 commits into from
May 26, 2022
Merged

fix: Canvas jitters on page refresh on an application where widgets don't occupy the entire width of the canvas #13917

merged 6 commits into from
May 26, 2022

Conversation

jsartisan
Copy link
Contributor

@jsartisan jsartisan commented May 18, 2022 •
edited by github-actions bot

When there are widgets on the canvas, on refresh the width of the widget flickers due to a bug in dynamic app layout logic. This PR fixes that

Fixes #12292

Type of chang

  • Bug fix

How Has This Been Tested?

  • Manually tested

Checklist:

  • 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

Test coverage results 🧪

🟢 Total coverage has increased 
// Code coverage diff between base branch:release and head branch: fix/flickering-of-canvas-width
Status File % Stmts % Branch % Funcs % Lines
🟢 total 56.64 (0) 38.61 (0.01) 35.96 (0) 56.9 (0.01)
🟢 app/client/src/pages/Editor/WidgetsEditor/CanvasContainer.tsx 100 (2.38) 77.78 (15.28) 100 (0) 100 (2.44)
🔴 app/client/src/pages/Editor/WidgetsEditor/index.tsx 93.55 (-0.2) 60 (0) 100 (0) 93.22 (-0.22)
🟢 app/client/src/reducers/uiReducers/mainCanvasReducer.ts 75 (2.27) 22.22 (0) 50 (0) 75 (2.27)
✨ 🆕 app/client/src/selectors/mainCanvasSelectors.tsx 100 100 100 100
🟢 app/client/src/utils/WorkerUtil.ts 89.76 (0) 72.55 (1.96) 100 (0) 93.33 (0)
🔴 app/client/src/utils/hooks/useDynamicAppLayout.tsx 86.84 (-1.47) 62.5 (-3.21) 100 (0) 90 (0)

@vercel
Copy link

vercel bot commented May 18, 2022 •
edited

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

Name Status Preview Updated
appsmith ✅ Ready (Inspect) Visit Preview May 19, 2022 at 10:12AM (UTC)

@jsartisan jsartisan changed the title fixes the flicker of widgets fix: Canvas jitters on page refresh on an application where widgets don't occupy the entire width of the canvas May 18, 2022
@jsartisan jsartisan requested a review from yaldram May 18, 2022 05:06
@github-actions github-actions bot added Bug Something isn't working High This issue blocks a user from building or impacts a lot of users Needs Triaging Needs attention from maintainers to triage Release UI Builders Pod Issues that UI Builders face using appsmith UI Building Pod UX Improvement and removed Bug Something isn't working labels May 18, 2022
@github-actions
Copy link

Unable to find test scripts. Please add necessary tests to the PR.

@github-actions github-actions bot added the Bug Something isn't working label May 18, 2022
@github-actions
Copy link

Unable to find test scripts. Please add necessary tests to the PR.

1 similar comment
@github-actions
Copy link

Unable to find test scripts. Please add necessary tests to the PR.

yaldram
yaldram suggested changes May 18, 2022
View reviewed changes
app/client/src/utils/hooks/useDynamicAppLayout.tsx Outdated
});

return initialized;
Copy link
Contributor

Choose a reason for hiding this comment

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

we can return a clean up function return () => { observer.disconnect() }

app/client/src/utils/hooks/useDynamicAppLayout.tsx Outdated
});

Copy link
Contributor

Choose a reason for hiding this comment

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

Also we can pass an empty dependency array to useEffect []

@jsartisan jsartisan requested a review from yaldram May 18, 2022 14:25
yaldram
yaldram previously approved these changes May 18, 2022
View reviewed changes
@jsartisan
Copy link
Contributor Author

/ok-to-test sha=8c75809

@github-actions
Copy link

Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/2349820930.
Workflow: Appsmith External Integration Test Workflow.
Commit: 8c75809.
PR: 13917.

@jsartisan jsartisan self-assigned this May 19, 2022
@jsartisan
Copy link
Contributor Author

/ok-to-test sha=c72b3bb

@github-actions
Copy link

Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/2351230147.
Workflow: Appsmith External Integration Test Workflow.
Commit: c72b3bb.
PR: 13917.

@jsartisan
Copy link
Contributor Author

/ok-to-test sha=c72b3bb

@github-actions
Copy link

Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/2352640718.
Workflow: Appsmith External Integration Test Workflow.
Commit: c72b3bb.
PR: 13917.

@github-actions
Copy link

UI Performance test run logs and artifacts: https://github.com/appsmithorg/appsmith/actions/runs/2352640718.
Commit: c72b3bb.
Results:

Click to view performance test results

Run 1 Run 2 Run 3 Run 4 Run 5 Median Mean SD.Sample SD.Population
SELECT_WIDGET_MENU_OPEN
scripting 1943.05 1961.74 2015.55 2018.44 1924.31 1961.74 1972.62 2.16 1.93
painting 12.09 17.16 11.56 21.69 11.97 12.09 14.89 29.82 26.66
rendering 619.81 611.8 614.46 608.46 623.32 614.46 615.57 0.97 0.87
SELECT_WIDGET_SELECT_OPTION
scripting 373.07 297.73 270.78 260.11 275.64 275.64 295.47 15.40 13.77
painting 12.22 3.73 7.75 7.52 4.21 7.52 7.09 48.10 43.02
rendering 15.93 16 15.56 15.76 18.49 15.93 16.35 7.40 6.61

1 similar comment
@github-actions
Copy link

UI Performance test run logs and artifacts: https://github.com/appsmithorg/appsmith/actions/runs/2352640718.
Commit: c72b3bb.
Results:

Click to view performance test results

Run 1 Run 2 Run 3 Run 4 Run 5 Median Mean SD.Sample SD.Population
SELECT_WIDGET_MENU_OPEN
scripting 1943.05 1961.74 2015.55 2018.44 1924.31 1961.74 1972.62 2.16 1.93
painting 12.09 17.16 11.56 21.69 11.97 12.09 14.89 29.82 26.66
rendering 619.81 611.8 614.46 608.46 623.32 614.46 615.57 0.97 0.87
SELECT_WIDGET_SELECT_OPTION
scripting 373.07 297.73 270.78 260.11 275.64 275.64 295.47 15.40 13.77
painting 12.22 3.73 7.75 7.52 4.21 7.52 7.09 48.10 43.02
rendering 15.93 16 15.56 15.76 18.49 15.93 16.35 7.40 6.61

keyurparalkar
keyurparalkar approved these changes May 20, 2022
View reviewed changes
Copy link
Contributor

@keyurparalkar keyurparalkar left a comment

Choose a reason for hiding this comment

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

LGTM!

@rahulramesha rahulramesha mentioned this pull request May 24, 2022
Closed
1 task
@jsartisan
Copy link
Contributor Author

@vivekverma2312 can someone from qa team pick this up to test?

@btsgh
Copy link

btsgh commented May 25, 2022

Tested on vercel link and found to be fixed.

@jsartisan jsartisan merged commit 7c9135f into release May 26, 2022
@jsartisan jsartisan deleted the fix/flickering-of-canvas-width branch May 26, 2022 04:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@keyurparalkar keyurparalkar keyurparalkar approved these changes

@yaldram yaldram yaldram left review comments

Assignees

@jsartisan jsartisan

Labels
Bug Something isn't working High This issue blocks a user from building or impacts a lot of users Needs Triaging Needs attention from maintainers to triage Release UI Builders Pod Issues that UI Builders face using appsmith UX Improvement
Projects
None yet
Milestone
No milestone
4 participants
@jsartisan @btsgh @yaldram @keyurparalkar