chore: hydrate widgets properties from session on drop #32934
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
WalkthroughWalkthroughThe recent updates focus on enhancing widget reusability and appearance consistency across the application. Key changes include saving widget properties in session storage for future reuse, introducing a Changes
Recent Review DetailsConfiguration used: CodeRabbit UI Files selected for processing (12)
Files skipped from review due to trivial changes (1)
Files skipped from review as they are similar to previous changes (11)
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 (
|
github-actions
bot
added
the
skip-changelog
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/8827251237. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Comment on lines
139
to
186
| export function savePropertyInSessionStorageIfRequired(props: { | ||
| isReusable: boolean; | ||
| widgetProperties: any; | ||
| propertyName: string; | ||
| propertyValue: string; | ||
| }) { | ||
| const { isReusable, propertyName, propertyValue, widgetProperties } = props; | ||
|
|
||
| if (isReusable && isDynamicValue(propertyValue) === false) { | ||
| let widgetType = widgetProperties.type; | ||
| let widgetPropertyName = propertyName; | ||
|
|
||
| // in case of type is WDS_ICON_BUTTON_WIDGET, we need to use key WDS_BUTTON_WIDGET, reason being | ||
| // we want to reuse the property values of icon button for button as well when we create button widget on drop | ||
| if (widgetType === "WDS_ICON_BUTTON_WIDGET") { | ||
| widgetType = "WDS_BUTTON_WIDGET"; | ||
| } | ||
|
|
||
| // in case of type is WDS_INLINE_BUTTONS_WIDGET, we want to just store the property that is being changed, not the whole property path | ||
| if (widgetType === "WDS_INLINE_BUTTONS_WIDGET") { | ||
| widgetPropertyName = propertyName.split(".").pop() as string; | ||
| } | ||
|
|
||
| sessionStorage.setItem( | ||
| `${widgetType}.${widgetPropertyName}`, | ||
| propertyValue, | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
Review the implementation of savePropertyInSessionStorageIfRequired for potential improvements in handling different widget types and properties.
- if (widgetType === "WDS_ICON_BUTTON_WIDGET") {
+ if (widgetType === WidgetTypes.ICON_BUTTON) {
- if (widgetType === "WDS_INLINE_BUTTONS_WIDGET") {
+ if (widgetType === WidgetTypes.INLINE_BUTTONS) {Committable suggestion
鈥硷笍 IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
Suggested change
| export function savePropertyInSessionStorageIfRequired(props: { | |
| isReusable: boolean; | |
| widgetProperties: any; | |
| propertyName: string; | |
| propertyValue: string; | |
| }) { | |
| const { isReusable, propertyName, propertyValue, widgetProperties } = props; | |
| if (isReusable && isDynamicValue(propertyValue) === false) { | |
| let widgetType = widgetProperties.type; | |
| let widgetPropertyName = propertyName; | |
| // in case of type is WDS_ICON_BUTTON_WIDGET, we need to use key WDS_BUTTON_WIDGET, reason being | |
| // we want to reuse the property values of icon button for button as well when we create button widget on drop | |
| if (widgetType === "WDS_ICON_BUTTON_WIDGET") { | |
| widgetType = "WDS_BUTTON_WIDGET"; | |
| } | |
| // in case of type is WDS_INLINE_BUTTONS_WIDGET, we want to just store the property that is being changed, not the whole property path | |
| if (widgetType === "WDS_INLINE_BUTTONS_WIDGET") { | |
| widgetPropertyName = propertyName.split(".").pop() as string; | |
| } | |
| sessionStorage.setItem( | |
| `${widgetType}.${widgetPropertyName}`, | |
| propertyValue, | |
| ); | |
| } | |
| } | |
| export function savePropertyInSessionStorageIfRequired(props: { | |
| isReusable: boolean; | |
| widgetProperties: any; | |
| propertyName: string; | |
| propertyValue: string; | |
| }) { | |
| const { isReusable, propertyName, propertyValue, widgetProperties } = props; | |
| if (isReusable && isDynamicValue(propertyValue) === false) { | |
| let widgetType = widgetProperties.type; | |
| let widgetPropertyName = propertyName; | |
| // in case of type is WDS_ICON_BUTTON_WIDGET, we need to use key WDS_BUTTON_WIDGET, reason being | |
| // we want to reuse the property values of icon button for button as well when we create button widget on drop | |
| if (widgetType === WidgetTypes.ICON_BUTTON) { | |
| widgetType = "WDS_BUTTON_WIDGET"; | |
| } | |
| // in case of type is WDS_INLINE_BUTTONS_WIDGET, we want to just store the property that is being changed, not the whole property path | |
| if (widgetType === WidgetTypes.INLINE_BUTTONS) { | |
| widgetPropertyName = propertyName.split(".").pop() as string; | |
| } | |
| sessionStorage.setItem( | |
| `${widgetType}.${widgetPropertyName}`, | |
| propertyValue, | |
| ); | |
| } | |
| } |
Comment on lines
+220
to
+238
| // if buttonVariant and buttonColor values ar present in session storage, then we should use those values | ||
| const buttonVariantSessionValue = sessionStorage.getItem( | ||
| "WDS_INLINE_BUTTONS_WIDGET.buttonVariant", | ||
| ); | ||
| const buttonColorSessionValue = sessionStorage.getItem( | ||
| "WDS_INLINE_BUTTONS_WIDGET.buttonColor", | ||
| ); | ||
|
|
||
| groupButtons[newGroupButtonId] = { | ||
| ...groupButtons[newGroupButtonId], | ||
| buttonVariant: buttonVariantSessionValue || "filled", | ||
| buttonColor: buttonColorSessionValue || "accent", | ||
| }; | ||
|
|
||
| // if the widget is a WDS_INLINE_BUTTONS_WIDGET, and button already have filled button variant in groupButtons, | ||
| // then we should add a secondary button ( outlined button ) instead of simple button | ||
| const filledButtonVariant = groupButtonsArray.find( | ||
| (groupButton: any) => groupButton.buttonVariant === "filled", | ||
| ); | ||
|
|
||
| if (filledButtonVariant) { | ||
| groupButtons[newGroupButtonId] = { | ||
| ...groupButtons[newGroupButtonId], | ||
| buttonVariant: "outlined", | ||
| buttonVariant: buttonVariantSessionValue || "outlined", |
There was a problem hiding this comment.
Review the logic for retrieving and applying session storage values for buttonVariant and buttonColor. Ensure it correctly handles scenarios where session values are not present.
- const buttonVariantSessionValue = sessionStorage.getItem("WDS_INLINE_BUTTONS_WIDGET.buttonVariant");
- const buttonColorSessionValue = sessionStorage.getItem("WDS_INLINE_BUTTONS_WIDGET.buttonColor");
+ const buttonVariantSessionValue = sessionStorage.getItem(`${WidgetTypes.INLINE_BUTTONS}.buttonVariant`);
+ const buttonColorSessionValue = sessionStorage.getItem(`${WidgetTypes.INLINE_BUTTONS}.buttonColor`);Committable suggestion
鈥硷笍 IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
Suggested change
| // if buttonVariant and buttonColor values ar present in session storage, then we should use those values | |
| const buttonVariantSessionValue = sessionStorage.getItem( | |
| "WDS_INLINE_BUTTONS_WIDGET.buttonVariant", | |
| ); | |
| const buttonColorSessionValue = sessionStorage.getItem( | |
| "WDS_INLINE_BUTTONS_WIDGET.buttonColor", | |
| ); | |
| groupButtons[newGroupButtonId] = { | |
| ...groupButtons[newGroupButtonId], | |
| buttonVariant: buttonVariantSessionValue || "filled", | |
| buttonColor: buttonColorSessionValue || "accent", | |
| }; | |
| // if the widget is a WDS_INLINE_BUTTONS_WIDGET, and button already have filled button variant in groupButtons, | |
| // then we should add a secondary button ( outlined button ) instead of simple button | |
| const filledButtonVariant = groupButtonsArray.find( | |
| (groupButton: any) => groupButton.buttonVariant === "filled", | |
| ); | |
| if (filledButtonVariant) { | |
| groupButtons[newGroupButtonId] = { | |
| ...groupButtons[newGroupButtonId], | |
| buttonVariant: "outlined", | |
| buttonVariant: buttonVariantSessionValue || "outlined", | |
| // if buttonVariant and buttonColor values ar present in session storage, then we should use those values | |
| const buttonVariantSessionValue = sessionStorage.getItem( | |
| `${WidgetTypes.INLINE_BUTTONS}.buttonVariant`, | |
| ); | |
| const buttonColorSessionValue = sessionStorage.getItem( | |
| `${WidgetTypes.INLINE_BUTTONS}.buttonColor`, | |
| ); | |
| groupButtons[newGroupButtonId] = { | |
| ...groupButtons[newGroupButtonId], | |
| buttonVariant: buttonVariantSessionValue || "filled", | |
| buttonColor: buttonColorSessionValue || "accent", | |
| }; | |
| // if the widget is a WDS_INLINE_BUTTONS_WIDGET, and button already have filled button variant in groupButtons, | |
| // then we should add a secondary button ( outlined button ) instead of simple button | |
| const filledButtonVariant = groupButtonsArray.find( | |
| (groupButton: any) => groupButton.buttonVariant === "filled", | |
| ); | |
| if (filledButtonVariant) { | |
| groupButtons[newGroupButtonId] = { | |
| ...groupButtons[newGroupButtonId], | |
| buttonVariant: buttonVariantSessionValue || "outlined", |
Comment on lines
793
to
578
| /** | ||
| * retrieves the values from session storage for the widget properties | ||
| * for hydration of the widget when we create widget on drop | ||
| */ | ||
| export function getWidgetSessionValues(type: string) { | ||
| // we don't need to hydrate widget for following widgets | ||
| if (["WDS_INLINE_BUTTONS_WIDGET"].includes(type)) return; | ||
|
|
||
| let widgetType = type; | ||
| const configMap = WidgetFactory.widgetConfigMap.get(type); | ||
| const widgetSessionValues: any = {}; | ||
|
|
||
| // in case we are dropping WDS_ICON_BUTTON_WIDGET, we want to reuse the values of BUTTON_WIDGET | ||
| if (type === "WDS_ICON_BUTTON_WIDGET") { | ||
| widgetType = "WDS_BUTTON_WIDGET"; | ||
| } | ||
|
|
||
| for (const key in configMap) { | ||
| if (configMap[key]) { | ||
| const valueFromSession = sessionStorage.getItem(`${widgetType}.${key}`); | ||
|
|
||
| if (valueFromSession) { | ||
| widgetSessionValues[key] = valueFromSession; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return widgetSessionValues; | ||
| } |
There was a problem hiding this comment.
Review the implementation of getWidgetSessionValues for potential improvements in handling different widget types and properties.
- if (["WDS_INLINE_BUTTONS_WIDGET"].includes(type)) return;
+ if ([WidgetTypes.INLINE_BUTTONS].includes(type)) return;
- if (type === "WDS_ICON_BUTTON_WIDGET") {
+ if (type === WidgetTypes.ICON_BUTTON) {Committable suggestion
鈥硷笍 IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
Suggested change
| /** | |
| * retrieves the values from session storage for the widget properties | |
| * for hydration of the widget when we create widget on drop | |
| */ | |
| export function getWidgetSessionValues(type: string) { | |
| // we don't need to hydrate widget for following widgets | |
| if (["WDS_INLINE_BUTTONS_WIDGET"].includes(type)) return; | |
| let widgetType = type; | |
| const configMap = WidgetFactory.widgetConfigMap.get(type); | |
| const widgetSessionValues: any = {}; | |
| // in case we are dropping WDS_ICON_BUTTON_WIDGET, we want to reuse the values of BUTTON_WIDGET | |
| if (type === "WDS_ICON_BUTTON_WIDGET") { | |
| widgetType = "WDS_BUTTON_WIDGET"; | |
| } | |
| for (const key in configMap) { | |
| if (configMap[key]) { | |
| const valueFromSession = sessionStorage.getItem(`${widgetType}.${key}`); | |
| if (valueFromSession) { | |
| widgetSessionValues[key] = valueFromSession; | |
| } | |
| } | |
| } | |
| return widgetSessionValues; | |
| } | |
| /** | |
| * retrieves the values from session storage for the widget properties | |
| * for hydration of the widget when we create widget on drop | |
| */ | |
| export function getWidgetSessionValues(type: string) { | |
| // we don't need to hydrate widget for following widgets | |
| if ([WidgetTypes.INLINE_BUTTONS].includes(type)) return; | |
| let widgetType = type; | |
| const configMap = WidgetFactory.widgetConfigMap.get(type); | |
| const widgetSessionValues: any = {}; | |
| // in case we are dropping WDS_ICON_BUTTON_WIDGET, we want to reuse the values of BUTTON_WIDGET | |
| if (type === WidgetTypes.ICON_BUTTON) { | |
| widgetType = "WDS_BUTTON_WIDGET"; | |
| } | |
| for (const key in configMap) { | |
| if (configMap[key]) { | |
| const valueFromSession = sessionStorage.getItem(`${widgetType}.${key}`); | |
| if (valueFromSession) { | |
| widgetSessionValues[key] = valueFromSession; | |
| } | |
| } | |
| } | |
| return widgetSessionValues; | |
| } |
|
Deploy-Preview-URL: https://ce-32934.dp.appsmith.com |
jsartisan
force-pushed
the
chore/preserve-semantic-properties
branch
from
152ac84
to
1e113f9
Compare
marks0351
requested changes
Apr 25, 2024
| if (this.props.widgetProperties.type === "WDS_INLINE_BUTTONS_WIDGET") { | ||
| // if buttonVariant and buttonColor values ar present in session storage, then we should use those values | ||
| const buttonVariantSessionValue = sessionStorage.getItem( |
There was a problem hiding this comment.
@jsartisan I would suggest we create generic methods to fetch widget based default values modifiers for property controls and use them to manipulate the default values set in the property controls. this would keep the widget related logic to pick the deafult value within the widget. like how we do for pasting(pasteOperationChecks) in Anvil. coz soon we might wanna do this for any property of a widget that is set from any property contorl.
WDYT?
There was a problem hiding this comment.
@marks0351 @jsartisan Any reason we approved & merged this PR without resolving this comment? Was this suggestion no longer relevant? If yes, can we please ensure we update these comments on Github before resolving & merging the PR? It ensures that everyone is on the same page.
There was a problem hiding this comment.
@mohanarpit we had discussed to do this as a follow up task as it required more detailing. @jsartisan can you pls mention the task here.
| @@ -125,6 +125,7 @@ function* getChildWidgetProps( | |||
| ]); | |||
| const themeDefaultConfig = | |||
| WidgetFactory.getWidgetStylesheetConfigMap(type) || {}; | |||
| const widgetSessionValues = getWidgetSessionValues(type); | |||
There was a problem hiding this comment.
lets make sure these valyes are only used for Anvil
jsartisan
force-pushed
the
chore/preserve-semantic-properties
branch
from
449ec5d
to
a50624c
Compare
added 7 commits
May 1, 2024 10:22
jsartisan
force-pushed
the
chore/preserve-semantic-properties
branch
from
a50624c
to
fbe155c
Compare
marks0351
reviewed
May 3, 2024
| @@ -574,6 +579,15 @@ const PropertyControl = memo((props: Props) => { | |||
| // updating properties of a widget(s) should be done only once when property value changes. | |||
| // to make sure dsl updates are atomic which is a necessity for undo/redo. | |||
| onBatchUpdatePropertiesOfMultipleWidgets(allPropertiesToUpdates); | |||
|
|
|||
| savePropertyInSessionStorageIfRequired({ | |||
There was a problem hiding this comment.
I think we should take a dispatch call, listen for it from a saga and set the session only for Anvil layout?
WDYT
marks0351
reviewed
May 3, 2024
| widgetProperties, | ||
| } = props; | ||
|
|
||
| if (isReusable && isDynamicValue(propertyValue) === false) { |
There was a problem hiding this comment.
@jsartisan in the follow up task I am assuming this part can also be fetched from individual widgets
marks0351
approved these changes
May 3, 2024
marks0351
reviewed
May 3, 2024
| @@ -130,6 +142,7 @@ function* getChildWidgetProps( | |||
| widgetId: newWidgetId, | |||
| renderMode: RenderModes.CANVAS, | |||
| ...themeDefaultConfig, | |||
| ...widgetSessionValues, | |||
There was a problem hiding this comment.
we should be adding this only for Anvil
There was a problem hiding this comment.
So for now, I have used isReusable property on anvil widgets only. So we should be good.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR adds a ability for anvil widgets ( mainly buttons, icon buttons, heading, paragraph and inline buttons ) to use values from session on creation on drop.
/ok-to-test tags="@tag.Anvil"
Summary by CodeRabbit
New Features
Bug Fixes
WidgetCard.tsxto ensure proper class name generation.Enhancements
Tip
馃煝 馃煝 馃煝 All cypress tests have passed! 馃帀 馃帀 馃帀
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/8936335019
Commit: fbe155c
Cypress dashboard url: Click here!