chore: add code for paragraph-text morphing #36065
Conversation
WalkthroughThe recent changes enhance multiple widgets by introducing new properties, functions, and constants. Key updates include the addition of dependency tracking and update hooks for improved widget state management, as well as the establishment of constants for input types and font styles. These modifications collectively refine the configurability and operational behavior of the widgets within the application interface. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Widget
participant UpdateHook
User->>Widget: Change property
Widget->>UpdateHook: Trigger update
UpdateHook->>Widget: Apply changes
Widget->>User: Reflect updated state
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 using PR comments)
Other keywords and placeholders
Documentation and Community
|
|
/build-deploy-preview skip-tests=true |
github-actions
bot
added
the
skip-changelog
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/10677230388. |
|
This PR has increased the number of cyclic dependencies by 1, when compared with the release branch. Refer this document to identify the cyclic dependencies introduced by this PR. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (2)
app/client/src/widgets/wds/WDSParagraphWidget/helpers.ts (1)
6-27: Great work on thefontSizeUpdateHookfunction! The code is clean and well-structured.A few observations and suggestions:
- The function signature is correctly typed and follows the expected pattern.
- Using a constant for mapping font size to widget type is a good practice as it keeps the mapping in a single place and makes it easy to update.
- Consider adding error handling for cases where the font size is not present in the mapping. This will make the function more robust and prevent unexpected behavior.
Here's an example of how you can add error handling:
updates.push({ propertyPath: "type", - propertyValue: - FONT_SIZE_TO_WIDGET_TYPE_MAP[ - propertyValue as keyof typeof FONT_SIZE_TO_WIDGET_TYPE_MAP - ], + propertyValue: (() => { + const widgetType = FONT_SIZE_TO_WIDGET_TYPE_MAP[ + propertyValue as keyof typeof FONT_SIZE_TO_WIDGET_TYPE_MAP + ]; + if (!widgetType) { + console.error(`No widget type found for font size: ${propertyValue}`); + return props.type; + } + return widgetType; + })(), });This will log an error if the font size is not present in the mapping and fallback to the current widget type.
Keep up the good work!
app/client/src/widgets/wds/WDSBaseInputWidget/helpers.ts (1)
6-40: Great job on theisReadOnlyUpdateHookfunction! The code is well-structured and follows a clear logic.A few suggestions for improvement:
Consider adding JSDoc comments to document the function's purpose, parameters, and return value. This will make it easier for other developers to understand and use the function.
Consider extracting the array of widget types that are excluded from the
readOnlyupdate logic into a constant. This will make the code more readable and maintainable.Consider adding unit tests to ensure that the function behaves as expected for different input scenarios.
Overall, the code changes look good to me!
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (5)
- app/client/src/widgets/wds/WDSBaseInputWidget/config/contentConfig.ts (2 hunks)
- app/client/src/widgets/wds/WDSBaseInputWidget/helpers.ts (1 hunks)
- app/client/src/widgets/wds/WDSParagraphWidget/config/propertyPaneConfig/styleConfig.ts (2 hunks)
- app/client/src/widgets/wds/WDSParagraphWidget/constants.ts (1 hunks)
- app/client/src/widgets/wds/WDSParagraphWidget/helpers.ts (1 hunks)
Files skipped from review due to trivial changes (1)
- app/client/src/widgets/wds/WDSParagraphWidget/constants.ts
Additional comments not posted (4)
app/client/src/widgets/wds/WDSParagraphWidget/config/propertyPaneConfig/styleConfig.ts (2)
4-4: Great job importing thefontSizeUpdateHook! 👍The import statement looks good. It's using the correct syntax and the relative import path seems to be pointing to the right location.
38-38: Excellent addition of theupdateHookproperty! 🌟Adding the
updateHookproperty to thefontSizeconfiguration is a great way to enhance the widget's responsiveness to font size changes.The
updateHookwill be invoked whenever thefontSizeproperty is updated, allowing you to perform any necessary actions or transformations based on the new font size value.This is a clean and effective way to handle font size updates dynamically. Well done!
app/client/src/widgets/wds/WDSBaseInputWidget/config/contentConfig.ts (2)
3-3: The import statement looks good.The
isReadOnlyUpdateHookfunction is correctly imported from the../helpersfile and is used in the configuration array.
125-126: Great job adding thedependenciesandupdateHookproperties!The
dependenciesproperty correctly specifies that bothtypeandinputTypeproperties are related. This will help manage the dependencies between these properties.The
updateHookproperty is set to the importedisReadOnlyUpdateHookfunction, which will likely manage updates in a read-only context. This will improve the functionality of the widget by handling updates appropriately based on the read-only state.
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range, codebase verification and nitpick comments (3)
app/client/src/widgets/wds/WDSParagraphWidget/helpers.ts (1)
6-27: Great job on thefontSizeUpdateHookfunction! The code is well-structured and follows best practices. 👏A few additional suggestions to consider:
You could add a comment above the function to explain its purpose and the expected input and output. This will make it easier for other developers to understand and use the function.
You could add some error handling to the function to handle cases where the
propertyValueis not a valid key in theFONT_SIZE_TO_WIDGET_TYPE_MAP. This will make the function more robust and prevent unexpected errors.You could use object destructuring to extract the
propertyNameandpropertyValuefrom thepropsobject. This will make the code more readable and concise.Here's an example of how you could implement these suggestions:
/** * Updates the font size and widget type based on the provided property value. * @param props The widget props. * @param propertyName The name of the property to update. * @param propertyValue The value of the property to update. * @returns An array of property updates. */ export function fontSizeUpdateHook( props: WidgetProps, propertyName: string, propertyValue: string, ) { const { propertyName, propertyValue } = props; const updates: PropertyUpdates[] = [ { propertyPath: propertyName, propertyValue, }, ]; const widgetType = FONT_SIZE_TO_WIDGET_TYPE_MAP[propertyValue as keyof typeof FONT_SIZE_TO_WIDGET_TYPE_MAP]; if (widgetType) { updates.push({ propertyPath: "type", propertyValue: widgetType, }); } else { console.warn(`Invalid font size: ${propertyValue}`); } return updates; }Keep up the great work! Let me know if you have any questions or if there's anything else I can help with.
app/client/src/widgets/wds/WDSBaseInputWidget/helpers.ts (2)
6-40: Consider renaming the function togetReadOnlyUpdateHookfor better clarity.The current function name
isReadOnlyUpdateHookis misleading as it doesn't return a boolean value. A more appropriate name would begetReadOnlyUpdateHookas it returns an array ofPropertyUpdates.
20-24: Extract the condition for checking the widget type into a separate variable for better readability.The condition for checking the widget type can be extracted into a separate variable to improve readability.
Apply this diff to extract the condition:
+ const isReadOnlyAllowed = !["WDS_CURRENCY_INPUT_WIDGET", "WDS_PHONE_INPUT_WIDGET"].includes( + props.type, + ); - if ( - !["WDS_CURRENCY_INPUT_WIDGET", "WDS_PHONE_INPUT_WIDGET"].includes( - props.type, - ) - ) { + if (isReadOnlyAllowed) {
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (5)
- app/client/src/widgets/wds/WDSBaseInputWidget/config/contentConfig.ts (2 hunks)
- app/client/src/widgets/wds/WDSBaseInputWidget/helpers.ts (1 hunks)
- app/client/src/widgets/wds/WDSParagraphWidget/config/propertyPaneConfig/styleConfig.ts (2 hunks)
- app/client/src/widgets/wds/WDSParagraphWidget/constants.ts (1 hunks)
- app/client/src/widgets/wds/WDSParagraphWidget/helpers.ts (1 hunks)
Files skipped from review due to trivial changes (1)
- app/client/src/widgets/wds/WDSParagraphWidget/constants.ts
Additional comments not posted (4)
app/client/src/widgets/wds/WDSParagraphWidget/config/propertyPaneConfig/styleConfig.ts (2)
4-4: Great job importing thefontSizeUpdateHook! 👍The import statement is correctly written and the imported hook is used in the
propertyPaneStyleConfigarray.
38-38: Excellent work adding theupdateHookproperty! 🌟The
updateHookproperty is correctly added to the object in thepropertyPaneStyleConfigarray. By assigning thefontSizeUpdateHookto theupdateHookproperty, you have enhanced the functionality of the property pane related to typography.This change allows for dynamic updates to the font size, making the widget more interactive and responsive. It's a great step towards improving the user experience and providing more flexibility in managing typography settings.
Keep up the good work! 😊
app/client/src/widgets/wds/WDSBaseInputWidget/config/contentConfig.ts (2)
3-3: Import looks good!The
isReadOnlyUpdateHookfunction is imported correctly from the../helpersmodule.
125-126: Great job adding thedependenciesandupdateHookproperties!The
dependenciesproperty correctly specifies that theisReadOnlyproperty depends on thetypeandinputTypeproperties. This dependency tracking will ensure that theisReadOnlyproperty is updated whenever the dependent properties change.The
updateHookproperty is set to theisReadOnlyUpdateHookfunction, which will be called whenever theisReadOnlyproperty is updated. This dedicated update mechanism allows for custom behavior when the widget's read-only state changes.These additions enhance the configurability and behavior of the widget, making it more robust and maintainable.
| if ( | ||
| !["WDS_CURRENCY_INPUT_WIDGET", "WDS_PHONE_INPUT_WIDGET"].includes( | ||
| props.type, | ||
| ) | ||
| ) { | ||
| if (propertyValue) { | ||
| updates.push({ | ||
| propertyPath: "type", | ||
| propertyValue: "WDS_KEY_VALUE_WIDGET", | ||
| }); | ||
| } else { | ||
| updates.push({ | ||
| propertyPath: "type", | ||
| propertyValue: | ||
| INPUT_TYPE_TO_WIDGET_TYPE_MAP[props.inputType as InputType], | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
Simplify the logic by using a ternary operator.
The if-else block can be simplified by using a ternary operator. This will make the code more concise and readable.
Apply this diff to simplify the logic:
- if (propertyValue) {
- updates.push({
- propertyPath: "type",
- propertyValue: "WDS_KEY_VALUE_WIDGET",
- });
- } else {
- updates.push({
- propertyPath: "type",
- propertyValue:
- INPUT_TYPE_TO_WIDGET_TYPE_MAP[props.inputType as InputType],
- });
- }
+ updates.push({
+ propertyPath: "type",
+ propertyValue: propertyValue
+ ? "WDS_KEY_VALUE_WIDGET"
+ : INPUT_TYPE_TO_WIDGET_TYPE_MAP[props.inputType as InputType],
+ });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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if ( | |
| !["WDS_CURRENCY_INPUT_WIDGET", "WDS_PHONE_INPUT_WIDGET"].includes( | |
| props.type, | |
| ) | |
| ) { | |
| if (propertyValue) { | |
| updates.push({ | |
| propertyPath: "type", | |
| propertyValue: "WDS_KEY_VALUE_WIDGET", | |
| }); | |
| } else { | |
| updates.push({ | |
| propertyPath: "type", | |
| propertyValue: | |
| INPUT_TYPE_TO_WIDGET_TYPE_MAP[props.inputType as InputType], | |
| }); | |
| } | |
| } | |
| if ( | |
| !["WDS_CURRENCY_INPUT_WIDGET", "WDS_PHONE_INPUT_WIDGET"].includes( | |
| props.type, | |
| ) | |
| ) { | |
| updates.push({ | |
| propertyPath: "type", | |
| propertyValue: propertyValue | |
| ? "WDS_KEY_VALUE_WIDGET" | |
| : INPUT_TYPE_TO_WIDGET_TYPE_MAP[props.inputType as InputType], | |
| }); | |
| } |
|
Deploy-Preview-URL: https://ce-36065.dp.appsmith.com |
|
This PR has increased the number of cyclic dependencies by 1, when compared with the release branch. Refer this document to identify the cyclic dependencies introduced by this PR. |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (12)
- app/client/src/widgets/wds/WDSBaseInputWidget/helpers.ts (1 hunks)
- app/client/src/widgets/wds/WDSBaseInputWidget/index.ts (1 hunks)
- app/client/src/widgets/wds/WDSBaseInputWidget/types.ts (1 hunks)
- app/client/src/widgets/wds/WDSEmailInputWidget/widget/index.tsx (1 hunks)
- app/client/src/widgets/wds/WDSInputWidget/component/index.tsx (1 hunks)
- app/client/src/widgets/wds/WDSInputWidget/component/types.ts (1 hunks)
- app/client/src/widgets/wds/WDSInputWidget/widget/helper.ts (1 hunks)
- app/client/src/widgets/wds/WDSInputWidget/widget/index.tsx (2 hunks)
- app/client/src/widgets/wds/WDSMultilineInputWidget/widget/index.tsx (1 hunks)
- app/client/src/widgets/wds/WDSNumberInputWidget/widget/index.tsx (1 hunks)
- app/client/src/widgets/wds/WDSPasswordInputWidget/widget/index.tsx (1 hunks)
- app/client/src/widgets/wds/WDSPhoneInputWidget/index.ts (1 hunks)
Files skipped from review due to trivial changes (3)
- app/client/src/widgets/wds/WDSBaseInputWidget/types.ts
- app/client/src/widgets/wds/WDSInputWidget/component/index.tsx
- app/client/src/widgets/wds/WDSInputWidget/widget/index.tsx
Files skipped from review as they are similar to previous changes (1)
- app/client/src/widgets/wds/WDSBaseInputWidget/helpers.ts
Additional comments not posted (13)
app/client/src/widgets/wds/WDSPhoneInputWidget/index.ts (1)
1-1: Great job enhancing the module's interface! 👍Exporting all members from the
./constantsmodule alongside theWDSPhoneInputWidgetcomponent is a wise decision. It improves the modularity of the code and makes it more convenient for consumers of this module to access the constants without needing separate imports.This change aligns with the best practice of exposing related resources together, which can lead to cleaner and more maintainable code in the long run. Well done!
app/client/src/widgets/wds/WDSBaseInputWidget/index.ts (1)
5-6: Great work on enhancing the module's interface! 👍The addition of the new export statements that include all exports from the
./typesand./constantsmodules is a fantastic way to improve the modularity and usability of theWDSBaseInputWidget. By consolidating the exports, you've made it easier for other parts of the codebase to import and use the necessary types and constants.The changes are consistent with the existing export statements and follow the best practices for module design. Keep up the excellent work in making the codebase more maintainable and user-friendly!
app/client/src/widgets/wds/WDSInputWidget/component/types.ts (2)
2-2: Great work on consolidating the type definitions! 👍Importing the
InputTypedirectly from theWDSBaseInputWidgetmodule is a cleaner approach and helps maintain consistency across the codebase.
4-4: Nice job reorganizing the import statements! 🙌Moving the
BaseInputComponentPropsimport to a separate line enhances readability and follows a logical order.app/client/src/widgets/wds/WDSEmailInputWidget/widget/index.tsx (1)
4-4: Verify the impact of the import change on theWDSEmailInputWidgetand other widgets.Great work on refactoring the widget hierarchy! The change in the import statement for
INPUT_TYPESsuggests a consolidation of input-related constants under a more generic base input widget.Please ensure that this change does not introduce any unintended side effects or break the functionality of the
WDSEmailInputWidgetor other widgets that rely on theINPUT_TYPESconstant.To verify the impact, you can run the following script:
If the tests pass without any issues, then the change is good to go! Let me know if you have any questions or need further assistance.
app/client/src/widgets/wds/WDSNumberInputWidget/widget/index.tsx (1)
5-5: Great work on refactoring the import path for theINPUT_TYPESconstant! This change promotes code reuse and maintainability.By moving the
INPUT_TYPESconstant to theWDSBaseInputWidgetlevel, you've made it accessible to all the widgets that extend theWDSBaseInputWidgetclass, including theWDSInputWidgetandWDSNumberInputWidgetclasses. This refactor helps to centralize the constants and reduces duplication across the widget hierarchy.Keep up the good work in identifying opportunities for code organization and reusability!
app/client/src/widgets/wds/WDSPasswordInputWidget/widget/index.tsx (2)
5-5: LGTM!Excellent work on organizing the import statements! Grouping related imports together improves the readability of the code.
3-3: Verify the usage ofINPUT_TYPESconstant.Great job refactoring the import path for
INPUT_TYPES! It's important to ensure that this constant is still being used correctly within the file after this change.Let's run the following script to verify the usage:
Verification successful
Let's try verifying the usage of
INPUT_TYPESagain by searching without specifying a file type. This should help us identify where and howINPUT_TYPESis used in the codebase.
Verification Successful: No Issues Found with
INPUT_TYPESImport ChangeThe change in the import path for
INPUT_TYPESinWDSPasswordInputWidget/widget/index.tsxdoes not affect its usage within the file, as there are no usages ofINPUT_TYPESfound there. The refactoring appears to be part of a broader reorganization and does not impact the functionality of theWDSPasswordInputWidget. Well done on maintaining the integrity of the code during refactoring!Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `INPUT_TYPES` constant. # Test: Search for the usage of `INPUT_TYPES`. Expect: Only valid usages. rg --type typescript -A 5 $'INPUT_TYPES'Length of output: 74
Script:
#!/bin/bash # Description: Verify the usage of `INPUT_TYPES` constant. # Test: Search for the usage of `INPUT_TYPES` in TypeScript files. rg 'INPUT_TYPES' --glob '*.ts' -A 5Length of output: 11482
app/client/src/widgets/wds/WDSMultilineInputWidget/widget/index.tsx (2)
5-5: LGTM!The reordering of the imports for
MultilineInputIconandMultilineInputThumbnailis a valid change. It improves the code organization and readability without affecting the widget's functionality.
4-4: Verify the impact of the import change on the widget's functionality.The import source for
INPUT_TYPEShas been updated fromwidgets/wds/WDSInputWidget/constantstowidgets/wds/WDSBaseInputWidget/constants. Let's ensure that this change does not introduce any breaking changes or affect the widget's behavior.To verify the impact, we can run the following script:
If the tests pass, it indicates that the import change is safe and does not introduce any breaking changes. However, if there are any discrepancies, please investigate further and ensure that the widget's functionality remains intact.
Verification successful
Import Change Verification Successful
The import change for
INPUT_TYPEStowidgets/wds/WDSBaseInputWidget/constantsis verified. The definition ofINPUT_TYPESexists in the new source, and there is no evidence of incorrect usage in theWDSMultilineInputWidgetclass. This suggests that the change does not introduce any breaking changes or affect the widget's behavior.
INPUT_TYPESis correctly defined inWDSBaseInputWidget/constants.ts.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of INPUT_TYPES in the codebase. # Test 1: Search for the usage of INPUT_TYPES in the WDSMultilineInputWidget. # Expect: The widget should still use the correct input type from the new import source. ast-grep --lang typescript --pattern $'class WDSMultilineInputWidget { $$$ static getDefaults() { return { $$$ inputType: INPUT_TYPES.$_, $$$ }; } $$$ }' # Test 2: Search for the definition of INPUT_TYPES in the new import source. # Expect: The new import source should define the same input types as the old source. rg --type typescript $'export const INPUT_TYPES' $(fd WDSBaseInputWidget/constants.ts)Length of output: 311
Script:
#!/bin/bash # Description: Re-verify the usage and definition of INPUT_TYPES in the codebase. # Test 1: Search for the usage of INPUT_TYPES in the WDSMultilineInputWidget. # Adjust the pattern to ensure it matches the correct usage. ast-grep --lang typescript --pattern $'class WDSMultilineInputWidget { $$$ static getDefaults() { return { $$$ inputType: INPUT_TYPES.$_, $$$ }; } $$$ }' # Test 2: Search for the definition of INPUT_TYPES in the new import source. # Remove the file type specification to avoid errors. rg 'export const INPUT_TYPES' $(fd WDSBaseInputWidget/constants.ts)Length of output: 426
app/client/src/widgets/wds/WDSInputWidget/widget/helper.ts (3)
11-11: Great work restructuring the import paths! 👍Changing the import path for
InputTypefrom a relative path to an absolute path improves the clarity and maintainability of the codebase. It aligns with the goal of consolidating related types within a centralized widget structure.The code change is approved. Keep up the good work!
15-18: Excellent job updating the import paths for the constants! 🌟Modifying the import path for
INPUT_TYPE_TO_WIDGET_TYPE_MAPandINPUT_TYPESto source them from the base widget module enhances the cohesiveness of the codebase. It aligns with the goal of grouping related functionalities together and reducing the complexity of managing multiple import paths.The code change is approved. Your efforts in improving the code structure are commendable!
20-20: Nice work simplifying the import statement! 🙌Removing the reference to
constantsand directly importinggetDefaultISDCodefromwidgets/wds/WDSPhoneInputWidgetstreamlines the import process. It aligns with the goal of improving the organizational structure and reducing complexity.The code change is approved. Your attention to detail in refining the imports is appreciated!
|
/build-deploy-preview skip-tests=true |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (3)
- app/client/src/widgets/wds/WDSInputWidget/widget/helper.ts (1 hunks)
- app/client/src/widgets/wds/WDSParagraphWidget/config/propertyPaneConfig/styleConfig.ts (2 hunks)
- app/client/src/widgets/wds/WDSParagraphWidget/constants.ts (1 hunks)
Files skipped from review due to trivial changes (1)
- app/client/src/widgets/wds/WDSInputWidget/widget/helper.ts
Files skipped from review as they are similar to previous changes (2)
- app/client/src/widgets/wds/WDSParagraphWidget/config/propertyPaneConfig/styleConfig.ts
- app/client/src/widgets/wds/WDSParagraphWidget/constants.ts
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/10678794575. |
|
Deploy-Preview-URL: https://ce-36065.dp.appsmith.com |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- app/client/src/widgets/wds/WDSParagraphWidget/config/defaultsConfig.ts (1 hunks)
Additional comments not posted (1)
app/client/src/widgets/wds/WDSParagraphWidget/config/defaultsConfig.ts (1)
14-14: Great work on adding thefontStyleproperty to enhance the widget's styling capabilities! 👍The addition of the
fontStyleproperty to thedefaultsConfigobject is a valuable enhancement that allows for further customization of theWDSParagraphWidget's appearance. By initializing it with an empty string value, you've made it an optional configuration option that can be easily customized later based on specific requirements.This change seamlessly integrates with the existing code structure and maintains consistency with the other properties in the
defaultsConfigobject. It expands the widget's styling flexibility without introducing any issues or altering the functionality of the existing properties.Keep up the excellent work in improving the configurability and versatility of the widgets! 🌟
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/10679329914. |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (2)
- app/client/src/layoutSystems/anvil/integrations/sagas/anvilWidgetAdditionSagas/helpers.ts (1 hunks)
- app/client/src/widgets/wds/WDSParagraphWidget/config/defaultsConfig.ts (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- app/client/src/widgets/wds/WDSParagraphWidget/config/defaultsConfig.ts
Additional comments not posted (1)
app/client/src/layoutSystems/anvil/integrations/sagas/anvilWidgetAdditionSagas/helpers.ts (1)
Line range hint
47-54: Verify the impact of removing the conditional check.The removal of the conditional check
if (configMap[key] !== undefined)alters the control flow of the function. It now processes all keys inconfigMap, regardless of whether their corresponding values areundefined.This change could lead to the generation of session storage keys for keys that previously would have been ignored, potentially affecting how widget session values are retrieved and stored.
To verify the impact, run the following script:
Additionally, consider adding a comment to document the new behavior:
// The function now processes all keys in `configMap`, regardless of whether // their corresponding values are `undefined`. This could lead to the generation // of session storage keys for keys that previously would have been ignored. for (const key in configMap) { // ... }
There was a problem hiding this comment.
Moved it to parent to avoid cyclic imports.
| @@ -43,7 +43,6 @@ export function getWidgetSessionValues( | |||
| } | |||
|
|
|||
| for (const key in configMap) { | |||
| if (configMap[key] === undefined) continue; | |||
There was a problem hiding this comment.
This is not required.
| @@ -11,6 +11,7 @@ export const defaultsConfig = { | |||
| fontSize: "body", | |||
| textAlign: "left", | |||
| textColor: "neutral", | |||
| fontStyle: undefined, | |||
There was a problem hiding this comment.
We need to add this, otherwise when hydrating values from session, we won't be able to get this key if not added here. Ideally WidgetFactory.widgetConfigMap.get(type) should return all keys with or without values. But for some reason, we are not getting fontStyle when the key is not added in the defualtsConfig. Noted this to check later.
| @@ -32,7 +34,7 @@ export const propertyPaneStyleConfig = [ | |||
| isJSConvertible: true, | |||
| isBindProperty: true, | |||
| isTriggerProperty: false, | |||
| isReusable: true, | |||
There was a problem hiding this comment.
isReusable is not required for fontSize as discussed with taras.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (2)
- app/client/src/widgets/wds/WDSParagraphWidget/constants.ts (1 hunks)
- app/client/src/widgets/wds/WDSParagraphWidget/helpers.ts (1 hunks)
Files skipped from review due to trivial changes (1)
- app/client/src/widgets/wds/WDSParagraphWidget/constants.ts
Files skipped from review as they are similar to previous changes (1)
- app/client/src/widgets/wds/WDSParagraphWidget/helpers.ts
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- app/client/src/widgets/wds/WDSParagraphWidget/tests/helpers.test.ts (1 hunks)
Additional comments not posted (1)
app/client/src/widgets/wds/WDSParagraphWidget/__tests__/helpers.test.ts (1)
4-38: Great job on the test case! The structure and assertions look good.The test case for the
fontSizeUpdateHookfunction is well-written and covers different scenarios. It tests the function with different font sizes and asserts that the function returns the correct updates for the font size and widget type.The use of the
expectfunction and thetoEqualmatcher is appropriate for comparing the expected and actual values.Keep up the good work!
| describe("isReadOnlyUpdateHook", () => { | ||
| it("should return the correct updates for input widget", () => { | ||
| const props1 = { | ||
| type: "WDS_INPUT_WIDGET", | ||
| inputType: "TEXT", | ||
| } as unknown as WidgetProps; | ||
|
|
||
| const updates = isReadOnlyUpdateHook(props1, "readOnly", true); | ||
| expect(updates).toEqual([ | ||
| { propertyPath: "readOnly", propertyValue: true }, | ||
| { propertyPath: "type", propertyValue: "WDS_KEY_VALUE_WIDGET" }, | ||
| ]); | ||
|
|
||
| const updates2 = isReadOnlyUpdateHook(props1, "readOnly", false); | ||
| expect(updates2).toEqual([ | ||
| { propertyPath: "readOnly", propertyValue: false }, | ||
| { propertyPath: "type", propertyValue: "WDS_INPUT_WIDGET" }, | ||
| ]); | ||
|
|
||
| const props2 = { | ||
| type: "WDS_EMAIL_INPUT_WIDGET", | ||
| inputType: "EMAIL", | ||
| } as unknown as WidgetProps; | ||
|
|
||
| const updates3 = isReadOnlyUpdateHook(props2, "readOnly", true); | ||
| expect(updates3).toEqual([ | ||
| { propertyPath: "readOnly", propertyValue: true }, | ||
| { propertyPath: "type", propertyValue: "WDS_KEY_VALUE_WIDGET" }, | ||
| ]); | ||
|
|
||
| const updates4 = isReadOnlyUpdateHook(props2, "readOnly", false); | ||
| expect(updates4).toEqual([ | ||
| { propertyPath: "readOnly", propertyValue: false }, | ||
| { propertyPath: "type", propertyValue: "WDS_EMAIL_INPUT_WIDGET" }, | ||
| ]); | ||
| }); | ||
|
|
||
| it("should not change the type for currency widget", () => { | ||
| const props = { | ||
| type: "WDS_CURRENCY_INPUT_WIDGET", | ||
| inputType: "CURRENCY", | ||
| } as unknown as WidgetProps; | ||
|
|
||
| const updates = isReadOnlyUpdateHook(props, "readOnly", true); | ||
| expect(updates).toEqual([ | ||
| { propertyPath: "readOnly", propertyValue: true }, | ||
| ]); | ||
| }); | ||
|
|
||
| it("should not change the type for phone widget", () => { | ||
| const props = { | ||
| type: "WDS_PHONE_INPUT_WIDGET", | ||
| inputType: "PHONE", | ||
| } as unknown as WidgetProps; | ||
|
|
||
| const updates = isReadOnlyUpdateHook(props, "readOnly", true); | ||
| expect(updates).toEqual([ | ||
| { propertyPath: "readOnly", propertyValue: true }, | ||
| ]); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Comprehensive Review of Test Cases for isReadOnlyUpdateHook
General Observations:
- The file introduces tests for the
isReadOnlyUpdateHookfunction, which appears to handle property updates based on widget type and readOnly status. - The tests are well-structured and cover multiple scenarios, including different widget types and readOnly states.
Detailed Observations:
-
Test Case for Input and Email Widgets (Lines 4-39):
- The tests correctly simulate different scenarios for input and email widgets, checking the updates when the readOnly property is toggled.
- The use of
as unknown as WidgetPropsfor type assertion is a bit forceful. Consider using a more refined type or mock that closely resemblesWidgetPropswithout resorting to unknown casting.
-
Test Case for Currency Widget (Lines 41-51):
- This test verifies that the type does not change when the widget is set to readOnly, which is a specific behavior presumably defined in the
isReadOnlyUpdateHookfunction. - It's good to see that the test expects no change in the type property, which aligns with the intended behavior described.
- This test verifies that the type does not change when the widget is set to readOnly, which is a specific behavior presumably defined in the
-
Test Case for Phone Widget (Lines 53-63):
- Similar to the currency widget test, this checks for no change in type when readOnly is true.
- Consistency in testing similar behaviors across different widget types is good for maintainability and understanding of the code.
Suggestions:
- Refine Type Assertions: Instead of using
as unknown as WidgetProps, create a more specific mock or utilize partials ofWidgetPropsto avoid broad type assertions. - Consider Additional Scenarios: Are there edge cases or error conditions that might affect the behavior of
isReadOnlyUpdateHook? If so, adding tests to cover these scenarios would be beneficial.
Overall:
- The tests are well-written and serve their purpose. However, minor improvements in type safety and possibly extending coverage could make them even better.
/ok-to-test tags="@tag.All" <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Summary by CodeRabbit - **New Features** - Enhanced configurability of input widgets with new dependency tracking and update mechanisms. - Introduced dynamic font size updates for paragraph widgets, improving typography control. - Expanded configuration options for paragraph widgets with a new `fontStyle` property. - Added a new type alias for input types, improving type safety and reliability. - Improved modularity by consolidating exports for better usability. - Introduced unit tests for font size update functionality, ensuring reliability. - Added unit tests for read-only update functionality, validating correct behavior across widget types. - **Bug Fixes** - Improved widget state management through the addition of read-only update hooks. <!-- end of auto-generated comment: release notes by coderabbit.ai --> <!-- This is an auto-generated comment: Cypress test results --> > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: <https://github.com/appsmithorg/appsmith/actions/runs/10719066708> > Commit: 395574a > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=10719066708&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.All` > Spec: > <hr>Thu, 05 Sep 2024 12:06:57 UTC <!-- end of auto-generated comment: Cypress test results --> --------- Co-authored-by: Pawan Kumar <[email protected]>
/ok-to-test tags="@tag.All"
Summary by CodeRabbit
Summary by CodeRabbit
New Features
fontStyleproperty.Bug Fixes
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/10719066708
Commit: 395574a
Cypress dashboard.
Tags:
@tag.AllSpec:
Thu, 05 Sep 2024 12:06:57 UTC