appsmith
appsmith copied to clipboard
appsmithorg
feat: Change default label position of RTE to top
Description
This PR changes the default label position of the Rich Text Editor widget from "Left" to "Top"
Fixes #15141
Type of change
- New feature (non-breaking change which adds functionality)
How Has This Been Tested?
- Manually tested
Checklist:
- [x] My code follows the style guidelines of this project
- [x] 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
- [x] My changes generate no new warnings
- [ ] I have added tests that prove my fix is effective or that my feature works
- [x] New and existing unit tests pass locally with my changes
Welcome to the Appsmith community! Thank you for your first pull request and making this project better. 🤗
![welcome[bot] avatar](https://avatars.githubusercontent.com/in/4141?v=4 "Published by a pub.dev verified publisher"){kind=small}
The latest updates on your projects. Learn more about Vercel for Git ↗︎
| Name | Status | Preview | Updated |
|---|---|---|---|
| appsmith | ✅ Ready (Inspect) | Visit Preview | Aug 8, 2022 at 6:09AM (UTC) |
![vercel[bot] avatar](https://avatars.githubusercontent.com/in/8329?v=4 "Published by a pub.dev verified publisher"){kind=small}
Unable to find test scripts. Please add necessary tests to the PR.
![github-actions[bot] avatar](https://avatars.githubusercontent.com/in/15368?v=4 "Published by a pub.dev verified publisher"){kind=small}
Unable to find test scripts. Please add necessary tests to the PR.
Have raised a WIP PR since Cypress tests were failing locally. Getting on a call with Nandan later for fixing that. Will remove the WIP tag then. cc: @somangshu
@dhruvikn WIP is used when a PR is raised but not ready for review, you can also use the create as a draft PR for the same.
If cypress tests are related to the PR itself then it can remain in progress while you work on it.
Also while checking the code out, I felt this property is stored in the DSL, my question is what happens to the widget that are already on the screen, Do we need to migrate them as well cc @dilippitchika
@dhruvikn another challenge I am noticing is that the rte widget is aligning itself in the vertical center, which is different from what is observed in the release env
PR
Release
Seems like the change pointed out here was intended, Dhruvik will find where this is happening. Once we merged the latest release the behaviour was similar.
This is the change that's causing this.
Line 100 (app/client/src/components/ads/LabelWithTooltip.tsx) in PR #15329
cc: @somangshu @jsartisan
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/2803258389.
Workflow: Appsmith External Integration Test Workflow.
Commit: 9744f01.
PR: 15769.
UI Performance test run logs and artifacts: https://github.com/appsmithorg/appsmith/actions/runs/2803258389.
Commit: 9744f01.
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 | 1066.62 | 1070.55 | 1040.73 | 1052.39 | 1024.51 | 1052.39 | 1050.96 | 1.80 | 1.61 |
| painting | 5.16 | 4.76 | 5.4 | 9.85 | 5.14 | 5.16 | 6.06 | 35.15 | 31.52 |
| rendering | 841.6 | 812.76 | 823.52 | 828.54 | 815.74 | 823.52 | 824.43 | 1.39 | 1.24 |
| SELECT_WIDGET_SELECT_OPTION | |||||||||
| scripting | 151.93 | 161.47 | 166.26 | 148.07 | 157.54 | 157.54 | 157.05 | 4.63 | 4.14 |
| painting | 5 | 2.2 | 6.09 | 2.09 | 4.47 | 4.47 | 3.97 | 44.58 | 39.80 |
| rendering | 313.21 | 309.41 | 311.31 | 306.4 | 311.14 | 311.14 | 310.29 | 0.83 | 0.74 |
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/2815762810.
Workflow: Appsmith External Integration Test Workflow.
Commit: e332657.
PR: 15769.
No migrations, only a change in default config of the widget @somangshu
UI Performance test run logs and artifacts: https://github.com/appsmithorg/appsmith/actions/runs/2815762810.
Commit: e332657.
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 | 1191.46 | 1047.93 | 1083.8 | 1148.63 | 1125.25 | 1125.25 | 1119.41 | 4.98 | 4.46 |
| painting | 19.1 | 12.63 | 27.99 | 6.76 | 9.82 | 12.63 | 15.26 | 55.37 | 49.54 |
| rendering | 1007.15 | 826.04 | 863.52 | 860.89 | 845 | 860.89 | 880.52 | 8.22 | 7.35 |
| SELECT_WIDGET_SELECT_OPTION | |||||||||
| scripting | 152.03 | 163.99 | 160.82 | 151.61 | 150.22 | 152.03 | 155.73 | 4.00 | 3.58 |
| painting | 3.77 | 6.24 | 6.04 | 3.41 | 4.58 | 4.58 | 4.81 | 26.82 | 23.91 |
| rendering | 308.53 | 309.28 | 298.8 | 305.57 | 297.45 | 305.57 | 303.93 | 1.81 | 1.62 |
Tested and verified. P.S Label's intended position behaviour would be on top (for left alignment) due to flex-start. cc: @dhruvikn
Let's merge, please, since it's QA approved and UI tests passed, but not reported. @somangshu. TIA.
@laveena-en what do you mean
P.S Label's intended position behaviour would be on top (for left alignment) due to flex-start.
![allcontributors[bot] avatar](https://avatars.githubusercontent.com/in/23186?v=4 "Published by a pub.dev verified publisher"){kind=small}