appsmith
appsmith copied to clipboard
appsmithorg
[WIP] - feat: Dynamic Menu Items
Description
Please include a summary of the changes and which issue has been fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.
Fixes #12362
Type of change
- New feature (non-breaking change which adds functionality)
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions, so we can reproduce. Please also list any relevant details for your test configuration.
- Test A
- Test B
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
The latest updates on your projects. Learn more about Vercel for Git ↗︎
| Name | Status | Preview | Updated |
|---|---|---|---|
| appsmith | ✅ Ready (Inspect) | Visit Preview | Nov 30, 2022 at 6:41PM (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}
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/3332953148.
Workflow: Appsmith External Integration Test Workflow.
Commit: ceadbaa.
PR: 17652.
Perf tests will be available at https://app.appsmith.com/app/performance-infra-dashboard/pr-details-63465d4789020c7ac296d08d?pr=17652&runId=3332953148_1
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/3333202007.
Workflow: Appsmith External Integration Test Workflow.
Commit: 1d3719d.
PR: 17652.
Perf tests will be available at https://app.appsmith.com/app/performance-infra-dashboard/pr-details-63465d4789020c7ac296d08d?pr=17652&runId=3333202007_1
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/3333363059.
Workflow: Appsmith External Integration Test Workflow.
Commit: 03673fe.
PR: 17652.
Perf tests will be available at https://app.appsmith.com/app/performance-infra-dashboard/pr-details-63465d4789020c7ac296d08d?pr=17652&runId=3333363059_1
Hey @dhruvikn Please have a look at the QA callouts mentioned below:
-The Array Item button continues to stay in the "hovered upon" state on navigating back to the main menu.
https://user-images.githubusercontent.com/109572422/198266365-4f1cb865-348c-480d-ae72-7a47dc387605.mov
-On changing any of the values in the styles section via the JS toggle and switching it off, some unintended binding is seen.
https://user-images.githubusercontent.com/109572422/198267556-b7007b1c-d547-452d-af10-804a2336c1d0.mov
- The tooltip needs to contain a more comprehensive text to convey it's functionality to the user
- On switching between pages, the source data seems to have a red border (which is seen for a couple of seconds) indicating that the binding is invalid.
https://user-images.githubusercontent.com/109572422/198268179-78bf0884-5294-47e9-8691-53b3f17f54f0.mov
- On enabling the JS in the general section, default value is not seen and the toggle doesn’t persist the state that was entered (true/false in this case)
https://user-images.githubusercontent.com/109572422/198268759-d5f142b6-bd50-4458-a65a-bbfd45f522a5.mov
Also. @dhruvikn are we not replacing the dropdown with the button group for static/dynamic menu items as suggested in the design file?
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/3336862126.
Workflow: Appsmith External Integration Test Workflow.
Commit: 92b13d4.
PR: 17652.
Perf tests will be available at https://app.appsmith.com/app/performance-infra-dashboard/pr-details-63465d4789020c7ac296d08d?pr=17652&runId=3336862126_1
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/3341207270.
Workflow: Appsmith External Integration Test Workflow.
Commit: fd3e456.
PR: 17652.
Perf tests will be available at https://app.appsmith.com/app/performance-infra-dashboard/pr-details-63465d4789020c7ac296d08d?pr=17652&runId=3341207270_1
Hey @laveena-en,
The Array Item button continues to stay in the "hovered upon" state on navigating back to the main menu.
This might be a side effect or the intended behavior from #15789. I have a workaround (specific to this button) but need to confirm first if this lies in the scope of the mentioned issue. Can you folks please take a look and let me know? @dilippitchika @hetunandu @rahulramesha
On changing any of the values in the styles section via the JS toggle and switching it off, some unintended binding is seen.
This has been addressed now.
However, another problem came out of this, specifically for the Icon property. As per my investigation, when toggling back from JS to normal, we can either set the icon to none or have a default value (any one-predetermined icon). This is also the observed behavior for any other similar place (can be observed in the table widget where it defaults to the "add" icon). Thoughts? @dilippitchika @sbalaji1192
If the video is not visible directly in this comment, please click here.
The tooltip needs to contain more comprehensive text to convey its functionality to the user
Addressed.
On switching between pages, the source data seems to have a red border (which is seen for a couple of seconds) indicating that the binding is invalid.
This is because you have a Query attached which runs on page load. Until that query's response is received, the source data is empty. Once the data is fetched, it goes back to normal.
If the video is not visible directly in this comment, please click here.
On enabling the JS in the general section, the default value is not seen and the toggle doesn’t persist in the state that was entered (true/false in this case).
Similar to JS toggle on the icon property, this seems to be an issue with custom property controls. I was able to observe similar behavior in the table widget as well. My initial investigation didn't reveal much. This seems like an issue that should be dealt with separately. Penny for your thoughts? @dilippitchika @sbalaji1192
If the video is not visible directly in this comment, please click here.
Also, are we not replacing the dropdown with the button group for static/dynamic menu items as suggested in the design file?
Thanks for pointing this out. When I started working on this issue, the tabs component was not present. Looks like #17454 was merged recently. Have addressed the change now.
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/3342091654.
Workflow: Appsmith External Integration Test Workflow.
Commit: 877f5ec.
PR: 17652.
Perf tests will be available at https://app.appsmith.com/app/performance-infra-dashboard/pr-details-63465d4789020c7ac296d08d?pr=17652&runId=3342091654_1
@dhruvikn @laveena-en @rahulramesha The Array Item button being focused on is part of Context Switching is should be reproducible on other such elements on release as well. This is not an issue
Regarding the JS conversion issues, please discuss with @aswathkk once @dhruvikn, these can be a regression as well. But agreed we need not fix it as part of this, if this is a bug which was already there, please create a new issue
Few feedbacks from me
- We should rename "Array item" to "Button configuration"
@sbalaji1192, have made the changes per your suggestions. Have also merged TABLE_PROPERTY and MENU_PROPERTY into one. Also, now we are not polluting the DSL with unnecessary properties related to dynamic menu items during migration and initial widget drop. Please review the PR once more. Thanks in advance. 🙌
@dilippitchika, have changed the button label to "Item Configuration" @jsartisan, have changed the color of the hint's curly braces as well.
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/3345880636.
Workflow: Appsmith External Integration Test Workflow.
Commit: 929f5f2.
PR: 17652.
Perf tests will be available at https://app.appsmith.com/app/performance-infra-dashboard/pr-details-63465d4789020c7ac296d08d?pr=17652&runId=3345880636_1
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/3345967065.
Workflow: Appsmith External Integration Test Workflow.
Commit: f25b794.
PR: 17652.
Perf tests will be available at https://app.appsmith.com/app/performance-infra-dashboard/pr-details-63465d4789020c7ac296d08d?pr=17652&runId=3345967065_1
@laveena-en @dilippitchika @dhruvikn I created a separate issue for the problem with JS toggle: https://github.com/appsmithorg/appsmith/issues/18008 I will take a look at it in the next sprint
Deployment failed with the following error:
Resource is limited - try again in 2 minutes (more than 100, code: "api-deployments-free-per-day").
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/3372968003.
Workflow: Appsmith External Integration Test Workflow.
Commit: ea43249.
PR: 17652.
Perf tests will be available at https://app.appsmith.com/app/performance-infra-dashboard/pr-details-63465d4789020c7ac296d08d?pr=17652&runId=3372968003_1