feat: On Canvas UI Colors and Widget API to consume these colors #33013
Conversation
…o feat/on-canvas-ui-colors
|
@riodeuno |
riodeuno
added
Anvil Pod
…o feat/on-canvas-ui-colors
riodeuno
added
ok-to-test
|
Warning Rate Limit Exceeded@riodeuno has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 0 minutes and 9 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
| @@ -114,6 +129,7 @@ class WidgetFactory { | |||
| needsHeightForContent: config.needsHeightForContent, | |||
| isSearchWildcard: config.isSearchWildcard, | |||
| needsErrorInfo: !!config.needsErrorInfo, | |||
| onCanvasUI, | |||
There was a problem hiding this comment.
I wonder why do we want it to be a part of widget config. since the config is used only by an editor component.
There was a problem hiding this comment.
I'm not sure what your concern is. Widgets owning configurations specific to the editor seem like a perfectly fine approach.
If you can share an alternative, it will help me understand your viewpoint.
There was a problem hiding this comment.
since the scope of using the config is only on the editor component and also for Anvil. The current widget name and how it figures out the parent all this will not be relavants with other layout systems which leads me to belive that we will keep widget name as is in those systems. which is why I think we should just use the getWidgetHierarchy to get the hierarchy of a widget and just set the color system of the widget name component.
There was a problem hiding this comment.
Yes. WidgetHierarchy is the most straightforward change.
However, there are multiple reasons for using an API approach.
- It may be used by UI modules for module widgets.
- It loosely coupled and keeps the ownership of configurations with the widgets.
The only con that I see is that it will store this configurations in the DSL, but for that we'll be picking up #21825 in the upcoming sprint.
There was a problem hiding this comment.
approving this.
however I still think the change in color codes is not based on individual widgets but is clearly based on hierarchy which exclusively only makes sense in Anvil.
|
|
||
| /** | ||
| * As this will make all layout system widgets have these properties. | ||
| * We're going to prioritise #21825. |
Description
Fixes #33075
Automation
/ok-to-test tags="@tag.Anvil"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/8897891359
Commit: 85cdd6b
Cypress dashboard url: Click here!
Communication
Should the DevRel and Marketing teams inform users about this change?