fix:JSONForm > Switch Field: label position has no effect, label wrap… #34298
Conversation
…s text unnecessarily appsmithorg#33793
WalkthroughThis update enhances the JSON form widget functionality by introducing a test specification for converting text input fields into switch fields. New locators improve UI interactions during tests, while modifications to key components allow for dynamic label positioning and alignment. Additionally, migration functions ensure that existing widgets are updated to meet the latest design specifications. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant JSONFormWidget
participant SwitchField
participant EntityExplorer
participant PropertyPane
User ->> EntityExplorer: Drag JSONForm widget to canvas
User ->> JSONFormWidget: Edit text input field
JSONFormWidget ->> PropertyPane: Open properties
User ->> PropertyPane: Change to switch field
PropertyPane ->> JSONFormWidget: Update field type
JSONFormWidget ->> SwitchField: Render switch field
User ->> SwitchField: Interact with switch
SwitchField ->> JSONFormWidget: Update form state
Poem
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
Early access features: disabledWe are currently testing the following features in early access:
Note:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (6)
- app/client/cypress/e2e/Regression/ClientSide/Widgets/JSONForm/JSONForm_EditSwitchFied_spec.js (1 hunks)
- app/client/cypress/fixtures/jsonFormDslSwitchWidgetSourceData.json (1 hunks)
- app/client/src/widgets/JSONFormWidget/component/Field.tsx (2 hunks)
- app/client/src/widgets/JSONFormWidget/component/FieldLabel.tsx (2 hunks)
- app/client/src/widgets/JSONFormWidget/fields/SwitchField.tsx (6 hunks)
- app/client/src/widgets/JSONFormWidget/widget/propertyConfig/properties/switch.ts (2 hunks)
Files skipped from review due to trivial changes (1)
- app/client/cypress/fixtures/jsonFormDslSwitchWidgetSourceData.json
Additional context used
Biome
app/client/src/widgets/JSONFormWidget/fields/SwitchField.tsx
[error] 99-99: The assignment should not be in an expression. (lint/suspicious/noAssignInExpressions)
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
Additional comments not posted (9)
app/client/src/widgets/JSONFormWidget/widget/propertyConfig/properties/switch.ts (3)
5-5: Ensure that theLabelPositionimport is correctly referenced and used throughout the file.
27-42: The addition of thelabelPositionproperty with options for "Left" and "Right" aligns with the PR's goal to allow dynamic label positioning. Consider adding a comment explaining the use ofLabelPositionenum for maintainability.
31-31: Using theLabelPositionenum ensures type safety and consistency. Good use of enum to standardize the values across the application.Also applies to: 38-38, 42-42
app/client/cypress/e2e/Regression/ClientSide/Widgets/JSONForm/JSONForm_EditSwitchFied_spec.js (1)
1-12: The updated import paths reflect the new file structure. Ensure that these paths are correct and that the imported modules are used appropriately in the tests.app/client/src/widgets/JSONFormWidget/component/Field.tsx (2)
26-26: The addition of "row-reverse" to thedirectiontype inStyledWrapperPropsis necessary for supporting the new label positioning functionality. Good forward compatibility.
69-75: The updated logic to determine the direction of the field based on thefieldClassNameandinlineLabelis clear and well-implemented. This change ensures that the label position can be dynamically set, aligning with the PR objectives.app/client/src/widgets/JSONFormWidget/fields/SwitchField.tsx (1)
14-14: The addition oflabelPositionto theSwitchComponentOwnPropsand its use in theSwitchComponentcall are correctly implemented. This change allows the label position to be dynamically set based on the form configuration, which is a key part of the fix for the issue addressed by this PR.Also applies to: 26-26, 40-40, 104-104, 113-113, 123-127
app/client/src/widgets/JSONFormWidget/component/FieldLabel.tsx (2)
34-34: The addition of "row-reverse" to thedirectionproperty enhances flexibility in label positioning. Good enhancement.
92-92: Setting thewidthtomax-contentin theStyledLabelTextcomponent is an effective way to prevent unnecessary text wrapping, aligning with the PR's objectives.
| describe( | ||
| "JSON Form Widget Custom Field", | ||
| { tags: ["@tag.Widget", "@tag.JSONForm"] }, | ||
| () => { | ||
| it("uses the custom field when the accessor matches", () => { | ||
| const formDsl = JSON.parse( | ||
| JSON.stringify(jsonFormDslSwitchWidgetSourceData), | ||
| ); | ||
|
|
||
| cy.addDsl(formDsl); | ||
|
|
||
| cy.openPropertyPane("jsonformwidget"); | ||
|
|
||
| // Add new custom field | ||
| cy.get(".t--add-column-btn") | ||
| .contains("Add new field") | ||
| .click({ force: true }); | ||
|
|
||
| cy.openFieldConfiguration("customField1"); | ||
|
|
||
| cy.testJsontext("propertyname", "education"); | ||
| cy.testJsontext("text", "Education"); | ||
| cy.selectDropdownValue(commonlocators.jsonFormFieldType, "Switch"); | ||
|
|
||
| // check for the position alignment | ||
| cy.get(commonlocators.optionposition).last().click({ force: true }); | ||
| cy.wait(200); | ||
| cy.get(commonlocators.optionpositionL).last().click({ force: true }); | ||
| cy.wait(200); | ||
| cy.closePropertyPane(); | ||
|
|
||
| const sourceData = { | ||
| name: "John", | ||
| email: "[email protected]", | ||
| }; | ||
|
|
||
| EditorNavigation.SelectEntityByName("JSONForm1", EntityType.Widget); | ||
| propPane.NavigateBackToPropertyPane(); | ||
| propPane.EnterJSContext("Source data", JSON.stringify(sourceData), true); | ||
|
|
||
| cy.get(`${fieldPrefix}-name label`).contains("Name"); | ||
| cy.get(`${fieldPrefix}-name input`).should("have.value", "John"); | ||
| }); |
There was a problem hiding this comment.
Tip
Codebase Verification
The search did not find any specific assertions verifying the label's position in the provided test scenario. Consider adding assertions to explicitly verify the label's position to ensure the label positioning functionality works as expected.
- Location:
app/client/cypress/e2e/Regression/ClientSide/Widgets/JSONForm/JSONForm_EditSwitchFied_spec.js
Analysis chain
This test scenario effectively covers the functionality of adding a custom switch field and verifying its alignment based on the label position. It's good to see detailed steps and assertions. Consider adding more assertions to verify the label's actual position to ensure the label positioning functionality works as expected.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify additional assertions for label position in the test scenario.
# Test: Check for assertions related to label position. Expect: Assertions that verify label position.
rg --type js $'cy.get.*labelPosition'
Length of output: 38
Script:
#!/bin/bash
# Description: Verify additional assertions for label position in the test scenario.
# Test: Check for assertions related to label position. Expect: Assertions that verify label position.
rg --type js 'cy.get.*label'
rg --type js 'cy.get.*position'
Length of output: 28745
There was a problem hiding this comment.
hello @coderabbitai the line no 41 and 42 helps in verifying the label position functionality works as expected as we are having both scenarios working correctly the assertion in this case would become redundant .
There was a problem hiding this comment.
@Harshithazemoso, thank you for pointing out lines 41 and 42 where the label position is being configured. Indeed, these lines are crucial for setting the label's position for the test scenario. However, they appear to be more about configuration rather than verification.
For robust automated testing, it's generally recommended to include explicit assertions that verify the final state against expected outcomes. This could be something like checking if the label is visually positioned as expected on the screen, which would make the tests more resilient to future changes affecting the label's position inadvertently.
If you believe the current tests sufficiently cover these aspects through other means, that's great! Otherwise, consider adding a couple of explicit assertions to ensure the label's position is as intended. If you need any help crafting these assertions, feel free to ask!
There was a problem hiding this comment.
@Harshithazemoso As per automation standard, we need to use expect conditions to "verifying the label position functionality".
Please review "Cypress Brest practices"
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| isSwitchedOn={value ?? false} | ||
| label="" | ||
| labelPosition={LabelPosition.Left} | ||
| labelPosition={schemaItem.labelPosition} |
There was a problem hiding this comment.
The assignment within the expression should be avoided as it can lead to confusion and potential bugs. Consider restructuring this part of the code to separate the assignment from the expression.
- labelPosition={schemaItem.labelPosition}
+ const labelPos = schemaItem.labelPosition;
+ labelPosition={labelPos}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.
| labelPosition={schemaItem.labelPosition} | |
| const labelPos = schemaItem.labelPosition; | |
| labelPosition={labelPos} |
|
|
||
| // check for the position alignment | ||
| cy.get(commonlocators.optionposition).last().click({ force: true }); | ||
| cy.wait(200); |
There was a problem hiding this comment.
Please don't add static wait but use appropriate assertions here
There was a problem hiding this comment.
hello @ApekshaBhosale made the changes can you review it now .
Thank you
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- app/client/cypress/e2e/Regression/ClientSide/Widgets/JSONForm/JSONForm_EditSwitchFied_spec.js (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- app/client/cypress/e2e/Regression/ClientSide/Widgets/JSONForm/JSONForm_EditSwitchFied_spec.js
|
hello @ApekshaBhosale ,I have made the changes requested ,could you please review the pr |
|
This PR has not seen activitiy for a while. It will be closed in 7 days unless further activity is detected. |
|
Hello @Nikhil-Nandagopal ,can you assign someone to review the pr ,It has been more than 3 weeks ,its still in review |
| cy.openPropertyPane("jsonformwidget"); | ||
|
|
||
| // Add new custom field | ||
| cy.get(".t--add-column-btn") |
There was a problem hiding this comment.
Can we please make this are locator and pass here?
|
|
||
| // check for the position alignment | ||
| cy.get(commonlocators.optionposition).last().click({ force: true }); | ||
| cy.get('.t--jsonformfield-customField1.switch') |
There was a problem hiding this comment.
This locator seems used for multiple places. Please make as locator variable and use.
| propPane, | ||
| } = require("../../../../../support/Objects/ObjectsCore"); | ||
|
|
||
| const fieldPrefix = ".t--jsonformfield"; |
There was a problem hiding this comment.
Please do not use locator as string variable.
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- app/client/cypress/e2e/Regression/ClientSide/Widgets/JSONForm/JSONForm_EditSwitchFied_spec.js (1 hunks)
- app/client/cypress/locators/commonlocators.json (1 hunks)
Additional context used
Path-based instructions (2)
app/client/cypress/e2e/Regression/ClientSide/Widgets/JSONForm/JSONForm_EditSwitchFied_spec.js (1)
Pattern
app/client/cypress/**/**.*: Follow best practices for Cypress code and e2e automation.
Avoid using cy.wait in code.
Avoid using cy.pause in code.
Use locator variables for locators and do not use plain strings.
Use data-* attributes for selectors.
Avoid Xpaths and CSS attributes.
Avoid selectors like .btn.submit or button[type=submit].
Perform logins via API withLoginFromAPI.
Avoid using it.only.
Use before, beforeEach, after, afterEach correctly.
Clean state before tests.
Use multiple assertions.
Don't treat Cypress as unit tests.
Use constants for strings in locator.
Include datasource operations inbeforehooks.
Do not use duplicate filenames.app/client/cypress/locators/commonlocators.json (1)
Pattern
app/client/cypress/**/**.*: Follow best practices for Cypress code and e2e automation.
Avoid using cy.wait in code.
Avoid using cy.pause in code.
Use locator variables for locators and do not use plain strings.
Use data-* attributes for selectors.
Avoid Xpaths and CSS attributes.
Avoid selectors like .btn.submit or button[type=submit].
Perform logins via API withLoginFromAPI.
Avoid using it.only.
Use before, beforeEach, after, afterEach correctly.
Clean state before tests.
Use multiple assertions.
Don't treat Cypress as unit tests.
Use constants for strings in locator.
Include datasource operations inbeforehooks.
Do not use duplicate filenames.
Additional comments not posted (4)
app/client/cypress/e2e/Regression/ClientSide/Widgets/JSONForm/JSONForm_EditSwitchFied_spec.js (3)
9-12: Follow Cypress best practices.Ensure the test scenario follows Cypress best practices, such as avoiding
cy.wait, using locator variables, and performing logins via API.
5-7: Ensure valid usage of modified imports.Verify the usage of
commonlocators,jsonFormDslSwitchWidgetSourceData, andpropPanefrom the updated file paths.Verification successful
Valid usage of modified imports confirmed.
The imports
commonlocators,jsonFormDslSwitchWidgetSourceData, andpropPaneare correctly utilized within the fileapp/client/cypress/e2e/Regression/ClientSide/Widgets/JSONForm/JSONForm_EditSwitchFied_spec.js.
commonlocatorsis used multiple times within the file.jsonFormDslSwitchWidgetSourceDatais used specifically within the file.propPaneis also utilized within the file.No issues were found with the imports.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of modified imports. # Test: Check for the usage of `commonlocators`, `jsonFormDslSwitchWidgetSourceData`, and `propPane`. Expect: Valid usage in the test cases. rg --type js 'commonlocators' rg --type js 'jsonFormDslSwitchWidgetSourceData' rg --type js 'propPane'Length of output: 229487
1-3: Validate the usage of imported modules.Ensure that
EditorNavigationandEntityTypefrom../../../../../support/Pages/EditorNavigationare used correctly in the test cases.Verification successful
Validate the usage of imported modules in the specific file.
Ensure that
EditorNavigationandEntityTypeare used correctly inapp/client/cypress/e2e/Regression/ClientSide/Widgets/JSONForm/JSONForm_EditSwitchFied_spec.js.
Verification successful: Imported modules are used correctly.
EditorNavigationandEntityTypeare correctly used inapp/client/cypress/e2e/Regression/ClientSide/Widgets/JSONForm/JSONForm_EditSwitchFied_spec.js.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of imported modules. # Test: Check for the usage of `EditorNavigation` and `EntityType`. Expect: Valid usage in the test cases. rg --type js 'EditorNavigation' rg --type js 'EntityType'Length of output: 220491
Script:
#!/bin/bash # Description: Verify the usage of imported modules in the specific file. # Check for the usage of `EditorNavigation` and `EntityType` in the target file. rg 'EditorNavigation' app/client/cypress/e2e/Regression/ClientSide/Widgets/JSONForm/JSONForm_EditSwitchFied_spec.js -A 5 -B 5 rg 'EntityType' app/client/cypress/e2e/Regression/ClientSide/Widgets/JSONForm/JSONForm_EditSwitchFied_spec.js -A 5 -B 5Length of output: 1858
app/client/cypress/locators/commonlocators.json (1)
237-238: Ensure correct usage of new locators.Verify that the new locators
jsonformFieldCustomFieldSwitchandjsonFieldPrefixare correctly defined and used in the test cases.Verification successful
The new locators are correctly defined and used in the test cases.
The locators
jsonformFieldCustomFieldSwitchandjsonFieldPrefixare used appropriately in theapp/client/cypress/e2e/Regression/ClientSide/Widgets/JSONForm/JSONForm_EditSwitchFied_spec.jsfile.
jsonformFieldCustomFieldSwitchis used in lines withcy.get(commonlocators.jsonformFieldCustomFieldSwitch).jsonFieldPrefixis used in lines withcy.get(\${commonlocators.jsonFieldPrefix} label`)andcy.get(`${commonlocators.jsonFieldPrefix} input`)`.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of new locators. # Test: Check for the usage of `jsonformFieldCustomFieldSwitch` and `jsonFieldPrefix`. Expect: Valid usage in the test cases. rg --type js 'jsonformFieldCustomFieldSwitch' rg --type js 'jsonFieldPrefix'Length of output: 726
| it("uses the custom field when the accessor matches", () => { | ||
| const formDsl = JSON.parse( | ||
| JSON.stringify(jsonFormDslSwitchWidgetSourceData), | ||
| ); | ||
|
|
||
| cy.addDsl(formDsl); | ||
|
|
||
| cy.openPropertyPane("jsonformwidget"); | ||
|
|
||
| // Add new custom field | ||
| cy.get(commonlocators.jsonFormAddNewCustomFieldBtn).click({ | ||
| force: true, | ||
| }); | ||
|
|
||
| cy.openFieldConfiguration("customField1"); | ||
|
|
||
| cy.testJsontext("propertyname", "education"); | ||
| cy.testJsontext("text", "Education"); | ||
| cy.selectDropdownValue(commonlocators.jsonFormFieldType, "Switch"); | ||
|
|
||
| // check for the position alignment | ||
| cy.get(commonlocators.optionposition).last().click({ force: true }); | ||
| cy.get(commonlocators.jsonformFieldCustomFieldSwitch).should( | ||
| "have.attr", | ||
| "direction", | ||
| "row-reverse", | ||
| ); | ||
| cy.get(commonlocators.optionpositionL).last().click({ force: true }); | ||
| cy.get(commonlocators.jsonformFieldCustomFieldSwitch).should( | ||
| "have.attr", | ||
| "direction", | ||
| "row", | ||
| ); | ||
| cy.closePropertyPane(); | ||
|
|
||
| const sourceData = { | ||
| name: "John", | ||
| email: "[email protected]", | ||
| }; | ||
|
|
||
| EditorNavigation.SelectEntityByName("JSONForm1", EntityType.Widget); | ||
| propPane.NavigateBackToPropertyPane(); | ||
| propPane.EnterJSContext("Source data", JSON.stringify(sourceData), true); | ||
|
|
||
| cy.get(`${commonlocators.jsonFieldPrefix} label`).contains("Name"); | ||
| cy.get(`${commonlocators.jsonFieldPrefix} input`).should( | ||
| "have.value", | ||
| "John", | ||
| ); | ||
| }); |
There was a problem hiding this comment.
Add explicit assertions for label position.
While the test scenario sets the label position, it lacks explicit assertions to verify the final state of the label's position. Add assertions to ensure the label's position is as expected.
+ // Assert label position is row-reverse
+ cy.get(commonlocators.jsonformFieldCustomFieldSwitch)
+ .should("have.css", "flex-direction", "row-reverse");
+ // Assert label position is row
+ cy.get(commonlocators.jsonformFieldCustomFieldSwitch)
+ .should("have.css", "flex-direction", "row");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.
| it("uses the custom field when the accessor matches", () => { | |
| const formDsl = JSON.parse( | |
| JSON.stringify(jsonFormDslSwitchWidgetSourceData), | |
| ); | |
| cy.addDsl(formDsl); | |
| cy.openPropertyPane("jsonformwidget"); | |
| // Add new custom field | |
| cy.get(commonlocators.jsonFormAddNewCustomFieldBtn).click({ | |
| force: true, | |
| }); | |
| cy.openFieldConfiguration("customField1"); | |
| cy.testJsontext("propertyname", "education"); | |
| cy.testJsontext("text", "Education"); | |
| cy.selectDropdownValue(commonlocators.jsonFormFieldType, "Switch"); | |
| // check for the position alignment | |
| cy.get(commonlocators.optionposition).last().click({ force: true }); | |
| cy.get(commonlocators.jsonformFieldCustomFieldSwitch).should( | |
| "have.attr", | |
| "direction", | |
| "row-reverse", | |
| ); | |
| cy.get(commonlocators.optionpositionL).last().click({ force: true }); | |
| cy.get(commonlocators.jsonformFieldCustomFieldSwitch).should( | |
| "have.attr", | |
| "direction", | |
| "row", | |
| ); | |
| cy.closePropertyPane(); | |
| const sourceData = { | |
| name: "John", | |
| email: "[email protected]", | |
| }; | |
| EditorNavigation.SelectEntityByName("JSONForm1", EntityType.Widget); | |
| propPane.NavigateBackToPropertyPane(); | |
| propPane.EnterJSContext("Source data", JSON.stringify(sourceData), true); | |
| cy.get(`${commonlocators.jsonFieldPrefix} label`).contains("Name"); | |
| cy.get(`${commonlocators.jsonFieldPrefix} input`).should( | |
| "have.value", | |
| "John", | |
| ); | |
| }); | |
| it("uses the custom field when the accessor matches", () => { | |
| const formDsl = JSON.parse( | |
| JSON.stringify(jsonFormDslSwitchWidgetSourceData), | |
| ); | |
| cy.addDsl(formDsl); | |
| cy.openPropertyPane("jsonformwidget"); | |
| // Add new custom field | |
| cy.get(commonlocators.jsonFormAddNewCustomFieldBtn).click({ | |
| force: true, | |
| }); | |
| cy.openFieldConfiguration("customField1"); | |
| cy.testJsontext("propertyname", "education"); | |
| cy.testJsontext("text", "Education"); | |
| cy.selectDropdownValue(commonlocators.jsonFormFieldType, "Switch"); | |
| // check for the position alignment | |
| cy.get(commonlocators.optionposition).last().click({ force: true }); | |
| cy.get(commonlocators.jsonformFieldCustomFieldSwitch).should( | |
| "have.attr", | |
| "direction", | |
| "row-reverse", | |
| ); | |
| + // Assert label position is row-reverse | |
| + cy.get(commonlocators.jsonformFieldCustomFieldSwitch) | |
| + .should("have.css", "flex-direction", "row-reverse"); | |
| cy.get(commonlocators.optionpositionL).last().click({ force: true }); | |
| cy.get(commonlocators.jsonformFieldCustomFieldSwitch).should( | |
| "have.attr", | |
| "direction", | |
| "row", | |
| ); | |
| + // Assert label position is row | |
| + cy.get(commonlocators.jsonformFieldCustomFieldSwitch) | |
| + .should("have.css", "flex-direction", "row"); | |
| cy.closePropertyPane(); | |
| const sourceData = { | |
| name: "John", | |
| email: "[email protected]", | |
| }; | |
| EditorNavigation.SelectEntityByName("JSONForm1", EntityType.Widget); | |
| propPane.NavigateBackToPropertyPane(); | |
| propPane.EnterJSContext("Source data", JSON.stringify(sourceData), true); | |
| cy.get(`${commonlocators.jsonFieldPrefix} label`).contains("Name"); | |
| cy.get(`${commonlocators.jsonFieldPrefix} input`).should( | |
| "have.value", | |
| "John", | |
| ); | |
| }); |
|
hello @sagar-qa007 made the changes as suggested could you please review the pr |
|
@Harshithazemoso can we run test case in cypress ? |
@sagar-qa007 sorry sagar I didnt get you ,are you asking can we test this spec here ? |
|
@Harshithazemoso can you please resolve merge conflicts here? |
This reverts commit 36d870d.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- app/client/cypress/locators/commonlocators.json (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- app/client/cypress/locators/commonlocators.json
@rahulbarwal I have resolved them |
| @@ -0,0 +1,64 @@ | |||
| import EditorNavigation, { | |||
There was a problem hiding this comment.
@Harshithazemoso we expect all new specs to be written in typescript. Can you please port this to TS?
There was a problem hiding this comment.
hello @rahulbarwal I have modified spec to .ts ,can you review it now
Thank you
|
@rahulbarwal updated as suggested , |
rahulbarwal
left a comment
rahulbarwal
left a comment
There was a problem hiding this comment.
Looks good to me.
Waiting for tests to pass here: #34958
@sagar-qa007 , @ApekshaBhosale Please review cypress changes.
@ashit-rath Please do a sanity check on migration code.
|
Hey @Harshithazemoso, |
@rahulbarwal can you mention the test cases ,they were working fine in my local |
@rahulbarwal can you please check now I have updated the failings cases |
There was a problem hiding this comment.
Actionable comments posted: 5
Outside diff range, codebase verification and nitpick comments (3)
app/client/cypress/e2e/Regression/ClientSide/Widgets/JSONForm/JSONForm_EditSwitchFied_spec.ts (1)
1-6: Group related imports.The imports from the same module (
ObjectsCore) can be grouped for better readability.- import { - agHelper, - draggableWidgets, - entityExplorer, - propPane, - } from "../../../../../support/Objects/ObjectsCore"; + import { agHelper, draggableWidgets, entityExplorer, propPane } from "../../../../../support/Objects/ObjectsCore";app/client/src/widgets/JSONFormWidget/widget/propertyConfig/properties/checkbox.ts (1)
5-6: Group related imports.The imports from the same module (
constants) can be grouped for better readability.- import { AlignWidgetTypes } from "WidgetProvider/constants"; - import { LabelPosition } from "components/constants"; + import { AlignWidgetTypes, LabelPosition } from "components/constants";app/client/src/widgets/JSONFormWidget/fields/switchField.test.tsx (1)
1-10: Group related imports.The imports from the same module (
constants) can be grouped for better readability.- import { LabelPosition } from "components/constants"; - import { AlignWidgetTypes } from "WidgetProvider/constants"; + import { LabelPosition, AlignWidgetTypes } from "components/constants";
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (11)
- app/client/cypress/e2e/Regression/ClientSide/Widgets/JSONForm/JSONForm_EditSwitchFied_spec.ts (1 hunks)
- app/client/packages/dsl/src/migrate/index.ts (2 hunks)
- app/client/packages/dsl/src/migrate/migrations/089-migrate-jsonformwidget-labelposition-and-alignment.ts (1 hunks)
- app/client/src/widgets/JSONFormWidget/component/Field.tsx (2 hunks)
- app/client/src/widgets/JSONFormWidget/component/FieldLabel.tsx (5 hunks)
- app/client/src/widgets/JSONFormWidget/fields/CheckboxField.tsx (4 hunks)
- app/client/src/widgets/JSONFormWidget/fields/SwitchField.tsx (4 hunks)
- app/client/src/widgets/JSONFormWidget/fields/switchField.test.tsx (1 hunks)
- app/client/src/widgets/JSONFormWidget/widget/propertyConfig/properties/checkbox.ts (2 hunks)
- app/client/src/widgets/JSONFormWidget/widget/propertyConfig/properties/switch.ts (2 hunks)
- app/client/src/widgets/SwitchWidget/component/index.tsx (1 hunks)
Files skipped from review as they are similar to previous changes (8)
- app/client/packages/dsl/src/migrate/index.ts
- app/client/packages/dsl/src/migrate/migrations/089-migrate-jsonformwidget-labelposition-and-alignment.ts
- app/client/src/widgets/JSONFormWidget/component/Field.tsx
- app/client/src/widgets/JSONFormWidget/component/FieldLabel.tsx
- app/client/src/widgets/JSONFormWidget/fields/CheckboxField.tsx
- app/client/src/widgets/JSONFormWidget/fields/SwitchField.tsx
- app/client/src/widgets/JSONFormWidget/widget/propertyConfig/properties/switch.ts
- app/client/src/widgets/SwitchWidget/component/index.tsx
Additional context used
Path-based instructions (1)
app/client/cypress/e2e/Regression/ClientSide/Widgets/JSONForm/JSONForm_EditSwitchFied_spec.ts (1)
Pattern
app/client/cypress/**/**.*: Review the following e2e test code written using the Cypress test library. Ensure that:
- Follow best practices for Cypress code and e2e automation.
- Avoid using cy.wait in code.
- Avoid using cy.pause in code.
- Avoid using agHelper.sleep().
- Use locator variables for locators and do not use plain strings.
- Use data-* attributes for selectors.
- Avoid Xpaths, Attributes and CSS path.
- Avoid selectors like .btn.submit or button[type=submit].
- Perform logins via API with LoginFromAPI.
- Perform logout via API with LogOutviaAPI.
- Perform signup via API with SignupFromAPI.
- Avoid using it.only.
- Avoid using after and aftereach in test cases.
- Use multiple assertions for expect statements.
- Avoid using strings for assertions.
- Do not use duplicate filenames even with different paths.
- Avoid using agHelper.Sleep, this.Sleep in any file in code.
Additional comments not posted (2)
app/client/src/widgets/JSONFormWidget/widget/propertyConfig/properties/checkbox.ts (2)
43-59: Add default value forlabelPosition.The
labelPositionproperty should have a default value to ensure consistent behavior.- defaultValue: LabelPosition.Left, + defaultValue: LabelPosition.Left,
61-79: Ensure consistency in property names and options.The
alignWidgetproperty should have consistent naming and options with other properties.- defaultValue: AlignWidgetTypes.LEFT, + defaultValue: AlignWidgetTypes.Left,
app/client/cypress/e2e/Regression/ClientSide/Widgets/JSONForm/JSONForm_EditSwitchFied_spec.ts
Show resolved
Hide resolved
app/client/cypress/e2e/Regression/ClientSide/Widgets/JSONForm/JSONForm_EditSwitchFied_spec.ts
Show resolved
Hide resolved
| const schemaItemRight = { | ||
| accessor: "Test Switch", | ||
| accentColor: "#553DE9", | ||
| alignWidget: AlignWidgetTypes.RIGHT, | ||
| boxShadow: "none", | ||
| children: {}, | ||
| dataType: DataType.STRING, | ||
| defaultValue: "", | ||
| fieldType: FieldType.SWITCH, | ||
| identifier: "TEST_IDENTIFIER", | ||
| isCustomField: false, | ||
| isDisabled: false, | ||
| isRequired: false, | ||
| isVisible: true, | ||
| label: "Test Switch", | ||
| labelPosition: LabelPosition.Right, | ||
| labelStyle: "", | ||
| labelTextColor: "", | ||
| labelTextSize: "0.875rem", | ||
| originalIdentifier: "Test Switch", | ||
| position: 0, | ||
| sourceData: "har", | ||
| tooltip: "", | ||
| }; |
There was a problem hiding this comment.
Avoid hardcoding mock data.
Consider using a factory function to generate mock data for better maintainability.
- const schemaItemRight = {
- accessor: "Test Switch",
- accentColor: "#553DE9",
- alignWidget: AlignWidgetTypes.RIGHT,
- boxShadow: "none",
- children: {},
- dataType: DataType.STRING,
- defaultValue: "",
- fieldType: FieldType.SWITCH,
- identifier: "TEST_IDENTIFIER",
- isCustomField: false,
- isDisabled: false,
- isRequired: false,
- isVisible: true,
- label: "Test Switch",
- labelPosition: LabelPosition.Right,
- labelStyle: "",
- labelTextColor: "",
- labelTextSize: "0.875rem",
- originalIdentifier: "Test Switch",
- position: 0,
- sourceData: "har",
- tooltip: "",
- };
+ const schemaItemRight = createMockSchemaItem({
+ alignWidget: AlignWidgetTypes.RIGHT,
+ labelPosition: LabelPosition.Right,
+ });Committable suggestion was skipped due to low confidence.
| const DefaultschemaItem = { | ||
| accessor: "Test Switch", | ||
| accentColor: "#553DE9", | ||
| alignWidget: AlignWidgetTypes.LEFT, | ||
| boxShadow: "none", | ||
| children: {}, | ||
| dataType: DataType.STRING, | ||
| defaultValue: "", | ||
| fieldType: FieldType.SWITCH, | ||
| identifier: "TEST_IDENTIFIER", | ||
| isCustomField: false, | ||
| isDisabled: false, | ||
| isRequired: false, | ||
| isVisible: true, | ||
| label: "Test Switch", | ||
| labelPosition: LabelPosition.Left, | ||
| labelStyle: "", | ||
| labelTextColor: "", | ||
| labelTextSize: "0.875rem", | ||
| originalIdentifier: "Test Switch", | ||
| position: 0, | ||
| sourceData: "har", | ||
| tooltip: "", | ||
| }; |
There was a problem hiding this comment.
Avoid hardcoding mock data.
Consider using a factory function to generate mock data for better maintainability.
- const DefaultschemaItem = {
- accessor: "Test Switch",
- accentColor: "#553DE9",
- alignWidget: AlignWidgetTypes.LEFT,
- boxShadow: "none",
- children: {},
- dataType: DataType.STRING,
- defaultValue: "",
- fieldType: FieldType.SWITCH,
- identifier: "TEST_IDENTIFIER",
- isCustomField: false,
- isDisabled: false,
- isRequired: false,
- isVisible: true,
- label: "Test Switch",
- labelPosition: LabelPosition.Left,
- labelStyle: "",
- labelTextColor: "",
- labelTextSize: "0.875rem",
- originalIdentifier: "Test Switch",
- position: 0,
- sourceData: "har",
- tooltip: "",
- };
+ const createMockSchemaItem = (overrides = {}) => ({
+ accessor: "Test Switch",
+ accentColor: "#553DE9",
+ alignWidget: AlignWidgetTypes.LEFT,
+ boxShadow: "none",
+ children: {},
+ dataType: DataType.STRING,
+ defaultValue: "",
+ fieldType: FieldType.SWITCH,
+ identifier: "TEST_IDENTIFIER",
+ isCustomField: false,
+ isDisabled: false,
+ isRequired: false,
+ isVisible: true,
+ label: "Test Switch",
+ labelPosition: LabelPosition.Left,
+ labelStyle: "",
+ labelTextColor: "",
+ labelTextSize: "0.875rem",
+ originalIdentifier: "Test Switch",
+ position: 0,
+ sourceData: "har",
+ tooltip: "",
+ ...overrides,
+ });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.
| const DefaultschemaItem = { | |
| accessor: "Test Switch", | |
| accentColor: "#553DE9", | |
| alignWidget: AlignWidgetTypes.LEFT, | |
| boxShadow: "none", | |
| children: {}, | |
| dataType: DataType.STRING, | |
| defaultValue: "", | |
| fieldType: FieldType.SWITCH, | |
| identifier: "TEST_IDENTIFIER", | |
| isCustomField: false, | |
| isDisabled: false, | |
| isRequired: false, | |
| isVisible: true, | |
| label: "Test Switch", | |
| labelPosition: LabelPosition.Left, | |
| labelStyle: "", | |
| labelTextColor: "", | |
| labelTextSize: "0.875rem", | |
| originalIdentifier: "Test Switch", | |
| position: 0, | |
| sourceData: "har", | |
| tooltip: "", | |
| }; | |
| const createMockSchemaItem = (overrides = {}) => ({ | |
| accessor: "Test Switch", | |
| accentColor: "#553DE9", | |
| alignWidget: AlignWidgetTypes.LEFT, | |
| boxShadow: "none", | |
| children: {}, | |
| dataType: DataType.STRING, | |
| defaultValue: "", | |
| fieldType: FieldType.SWITCH, | |
| identifier: "TEST_IDENTIFIER", | |
| isCustomField: false, | |
| isDisabled: false, | |
| isRequired: false, | |
| isVisible: true, | |
| label: "Test Switch", | |
| labelPosition: LabelPosition.Left, | |
| labelStyle: "", | |
| labelTextColor: "", | |
| labelTextSize: "0.875rem", | |
| originalIdentifier: "Test Switch", | |
| position: 0, | |
| sourceData: "har", | |
| tooltip: "", | |
| ...overrides, | |
| }); |
| describe("SwitchField", () => { | ||
| afterEach(() => { | ||
| jest.clearAllMocks(); | ||
| }); | ||
|
|
||
| it("renders the switch field with default props", () => { | ||
| render(<DefaultTestComponent />); | ||
| expect(screen.getByText("Test Switch")).toBeInTheDocument(); | ||
| }); | ||
|
|
||
| it("applies the correct label position and width for the switch widget", () => { | ||
| render(<DefaultTestComponent />); | ||
| const labelElement = screen.getByTestId("inlinelabel"); | ||
| expect(labelElement).toHaveStyle("width: 100%"); | ||
| }); | ||
| it("applies the correct label position", async () => { | ||
| render(<TestComponentRight />); | ||
| const labelElement = screen.getByTestId("inlinelabel"); | ||
| const switchComponent = screen.getByRole("checkbox"); | ||
|
|
||
| const switchComponentPosition = | ||
| switchComponent.compareDocumentPosition(labelElement); | ||
| expect(switchComponentPosition & Node.DOCUMENT_POSITION_FOLLOWING).toBe( | ||
| Node.DOCUMENT_POSITION_FOLLOWING, | ||
| ); | ||
| }); | ||
| it("applies the correct label position left and alignment right", () => { | ||
| render(<DefaultTestComponent />); | ||
|
|
||
| const switchComponent = screen.getByRole("checkbox"); | ||
| const labelElement = screen.getByText("Test Switch"); | ||
|
|
||
| const labelElementPosition = | ||
| labelElement.compareDocumentPosition(switchComponent); | ||
|
|
||
| expect(labelElementPosition & Node.DOCUMENT_POSITION_FOLLOWING).toBe( | ||
| Node.DOCUMENT_POSITION_FOLLOWING, | ||
| ); | ||
| }); |
There was a problem hiding this comment.
Add more assertions for comprehensive testing.
Ensure multiple assertions for each test case to cover different scenarios.
- it("renders the switch field with default props", () => {
- render(<DefaultTestComponent />);
- expect(screen.getByText("Test Switch")).toBeInTheDocument();
- });
+ it("renders the switch field with default props", () => {
+ render(<DefaultTestComponent />);
+ const switchField = screen.getByText("Test Switch");
+ expect(switchField).toBeInTheDocument();
+ expect(switchField).toBeVisible();
+ expect(switchField).toHaveClass("test-switch-field");
+ });
- it("applies the correct label position and width for the switch widget", () => {
- render(<DefaultTestComponent />);
- const labelElement = screen.getByTestId("inlinelabel");
- expect(labelElement).toHaveStyle("width: 100%");
- });
+ it("applies the correct label position and width for the switch widget", () => {
+ render(<DefaultTestComponent />);
+ const labelElement = screen.getByTestId("inlinelabel");
+ expect(labelElement).toHaveStyle("width: 100%");
+ expect(labelElement).toBeVisible();
+ });
- it("applies the correct label position", async () => {
- render(<TestComponentRight />);
- const labelElement = screen.getByTestId("inlinelabel");
- const switchComponent = screen.getByRole("checkbox");
- const switchComponentPosition = switchComponent.compareDocumentPosition(labelElement);
- expect(switchComponentPosition & Node.DOCUMENT_POSITION_FOLLOWING).toBe(Node.DOCUMENT_POSITION_FOLLOWING);
- });
+ it("applies the correct label position", async () => {
+ render(<TestComponentRight />);
+ const labelElement = screen.getByTestId("inlinelabel");
+ const switchComponent = screen.getByRole("checkbox");
+ const switchComponentPosition = switchComponent.compareDocumentPosition(labelElement);
+ expect(switchComponent).toBeVisible();
+ expect(labelElement).toBeVisible();
+ expect(switchComponentPosition & Node.DOCUMENT_POSITION_FOLLOWING).toBe(Node.DOCUMENT_POSITION_FOLLOWING);
+ });
- it("applies the correct label position left and alignment right", () => {
- render(<DefaultTestComponent />);
- const switchComponent = screen.getByRole("checkbox");
- const labelElement = screen.getByText("Test Switch");
- const labelElementPosition = labelElement.compareDocumentPosition(switchComponent);
- expect(labelElementPosition & Node.DOCUMENT_POSITION_FOLLOWING).toBe(Node.DOCUMENT_POSITION_FOLLOWING);
- });
+ it("applies the correct label position left and alignment right", () => {
+ render(<DefaultTestComponent />);
+ const switchComponent = screen.getByRole("checkbox");
+ const labelElement = screen.getByText("Test Switch");
+ const labelElementPosition = labelElement.compareDocumentPosition(switchComponent);
+ expect(switchComponent).toBeVisible();
+ expect(labelElement).toBeVisible();
+ expect(labelElementPosition & Node.DOCUMENT_POSITION_FOLLOWING).toBe(Node.DOCUMENT_POSITION_FOLLOWING);
+ });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.
| describe("SwitchField", () => { | |
| afterEach(() => { | |
| jest.clearAllMocks(); | |
| }); | |
| it("renders the switch field with default props", () => { | |
| render(<DefaultTestComponent />); | |
| expect(screen.getByText("Test Switch")).toBeInTheDocument(); | |
| }); | |
| it("applies the correct label position and width for the switch widget", () => { | |
| render(<DefaultTestComponent />); | |
| const labelElement = screen.getByTestId("inlinelabel"); | |
| expect(labelElement).toHaveStyle("width: 100%"); | |
| }); | |
| it("applies the correct label position", async () => { | |
| render(<TestComponentRight />); | |
| const labelElement = screen.getByTestId("inlinelabel"); | |
| const switchComponent = screen.getByRole("checkbox"); | |
| const switchComponentPosition = | |
| switchComponent.compareDocumentPosition(labelElement); | |
| expect(switchComponentPosition & Node.DOCUMENT_POSITION_FOLLOWING).toBe( | |
| Node.DOCUMENT_POSITION_FOLLOWING, | |
| ); | |
| }); | |
| it("applies the correct label position left and alignment right", () => { | |
| render(<DefaultTestComponent />); | |
| const switchComponent = screen.getByRole("checkbox"); | |
| const labelElement = screen.getByText("Test Switch"); | |
| const labelElementPosition = | |
| labelElement.compareDocumentPosition(switchComponent); | |
| expect(labelElementPosition & Node.DOCUMENT_POSITION_FOLLOWING).toBe( | |
| Node.DOCUMENT_POSITION_FOLLOWING, | |
| ); | |
| }); | |
| describe("SwitchField", () => { | |
| afterEach(() => { | |
| jest.clearAllMocks(); | |
| }); | |
| it("renders the switch field with default props", () => { | |
| render(<DefaultTestComponent />); | |
| const switchField = screen.getByText("Test Switch"); | |
| expect(switchField).toBeInTheDocument(); | |
| expect(switchField).toBeVisible(); | |
| expect(switchField).toHaveClass("test-switch-field"); | |
| }); | |
| it("applies the correct label position and width for the switch widget", () => { | |
| render(<DefaultTestComponent />); | |
| const labelElement = screen.getByTestId("inlinelabel"); | |
| expect(labelElement).toHaveStyle("width: 100%"); | |
| expect(labelElement).toBeVisible(); | |
| }); | |
| it("applies the correct label position", async () => { | |
| render(<TestComponentRight />); | |
| const labelElement = screen.getByTestId("inlinelabel"); | |
| const switchComponent = screen.getByRole("checkbox"); | |
| const switchComponentPosition = | |
| switchComponent.compareDocumentPosition(labelElement); | |
| expect(switchComponent).toBeVisible(); | |
| expect(labelElement).toBeVisible(); | |
| expect(switchComponentPosition & Node.DOCUMENT_POSITION_FOLLOWING).toBe( | |
| Node.DOCUMENT_POSITION_FOLLOWING, | |
| ); | |
| }); | |
| it("applies the correct label position left and alignment right", () => { | |
| render(<DefaultTestComponent />); | |
| const switchComponent = screen.getByRole("checkbox"); | |
| const labelElement = screen.getByText("Test Switch"); | |
| const labelElementPosition = | |
| labelElement.compareDocumentPosition(switchComponent); | |
| expect(switchComponent).toBeVisible(); | |
| expect(labelElement).toBeVisible(); | |
| expect(labelElementPosition & Node.DOCUMENT_POSITION_FOLLOWING).toBe( | |
| Node.DOCUMENT_POSITION_FOLLOWING, | |
| ); | |
| }); |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (4)
- app/client/src/widgets/JSONFormWidget/fields/switchField.test.tsx (1 hunks)
- app/client/src/widgets/JSONFormWidget/schemaParser.test.ts (3 hunks)
- app/client/src/widgets/JSONFormWidget/schemaTestData.ts (5 hunks)
- app/client/src/widgets/JSONFormWidget/widget/helper.test.ts (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- app/client/src/widgets/JSONFormWidget/fields/switchField.test.tsx
Additional comments not posted (11)
app/client/src/widgets/JSONFormWidget/widget/helper.test.ts (3)
1-1: Good practice: ImportingLabelPosition.Importing
LabelPositionfromcomponents/constantsis a good practice as it helps maintain consistency across the codebase.
409-409: Enhancement: AddedlabelPositionproperty toagreefield.Adding the
labelPositionproperty to theagreefield enhances the customization of the form's UI. Ensure that this property is tested to verify its behavior.
428-428: Enhancement: AddedlabelPositionproperty tofield1field.Adding the
labelPositionproperty to thefield1field enhances the customization of the form's UI. Ensure that this property is tested to verify its behavior.app/client/src/widgets/JSONFormWidget/schemaTestData.ts (5)
11-11: Good practice: ImportingLabelPosition.Importing
LabelPositionfromcomponents/constantsis a good practice as it helps maintain consistency across the codebase.
185-185: Enhancement: AddedlabelPositionproperty tobooleanfield.Adding the
labelPositionproperty to thebooleanfield enhances the customization of the form's UI. Ensure that this property is tested to verify its behavior.
380-380: Enhancement: AddedlabelPositionproperty tobooleanfield ineducationarray.Adding the
labelPositionproperty to thebooleanfield within theeducationarray enhances the customization of the form's UI. Ensure that this property is tested to verify its behavior.
802-802: Enhancement: AddedlabelPositionproperty tobooleanfield ineducationarray.Adding the
labelPositionproperty to thebooleanfield within theeducationarray enhances the customization of the form's UI. Ensure that this property is tested to verify its behavior.
1215-1215: Enhancement: AddedlabelPositionproperty tobooleanfield ineducationarray.Adding the
labelPositionproperty to thebooleanfield within theeducationarray enhances the customization of the form's UI. Ensure that this property is tested to verify its behavior.app/client/src/widgets/JSONFormWidget/schemaParser.test.ts (3)
29-29: Good job adding the necessary import forLabelPosition.This ensures that the
labelPositionproperty is correctly referenced in the tests.
215-215: Nice addition of thelabelPositionproperty to the schema item.This change ensures that the schema parsing logic correctly accounts for label positioning. Make sure to verify that this property is used consistently throughout the tests.
1087-1087: Good job adding thelabelPositionproperty to another schema item.This ensures comprehensive test coverage for label positioning. Ensure that this property is consistently used across all relevant test cases.
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/packages/dsl/src/migrate/index.ts (2 hunks)
- app/client/packages/dsl/src/migrate/tests/DSLMigration.test.ts (2 hunks)
- app/client/packages/dsl/src/migrate/tests/JsonFormSwitchCheckBoxPositionMigration.test.ts (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- app/client/packages/dsl/src/migrate/index.ts
Additional comments not posted (8)
app/client/packages/dsl/src/migrate/tests/JsonFormSwitchCheckBoxPositionMigration.test.ts (4)
1-3: Correct import statements.The import statements are correctly importing the necessary modules and types for the test.
4-115: Well-defined input DSL structure.The input DSL structure is well-defined and includes various properties and nested fields, which will be useful for testing the migration function.
117-228: Well-defined output DSL structure.The output DSL structure is well-defined and matches the expected result after the migration, which is crucial for validating the migration function.
230-235: Comprehensive test suite.The test suite is comprehensive and correctly checks if the migration function produces the expected output.
app/client/packages/dsl/src/migrate/tests/DSLMigration.test.ts (4)
Line range hint
1-91:
Correct import statements.The import statements are correctly importing the necessary modules and types for the test.
92-93: Correct import of the new migration module.The new migration module for version 89 has been correctly imported.
924-932: Correct addition of the new migration entry.The new migration entry for version 89 has been correctly added to the migration array.
Line range hint
933-1011:
Comprehensive test suite.The test suite is comprehensive and correctly ensures that all migrations are running correctly.
app/client/packages/dsl/src/migrate/tests/JsonFormSwitchCheckBoxPositionMigration.test.ts
Outdated
Show resolved
Hide resolved
| ${({ minHeight }) => ` | ||
| ${minHeight ? `min-height: ${minHeight}px;` : undefined}`}; | ||
| width: 100%; |
There was a problem hiding this comment.
Why is this not necessary now?
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/packages/dsl/src/migrate/tests/JsonFormSwitchCheckBoxPositionMigration.test.ts (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- app/client/packages/dsl/src/migrate/tests/JsonFormSwitchCheckBoxPositionMigration.test.ts
rahulbarwal
left a comment
rahulbarwal
left a comment
There was a problem hiding this comment.
LGTM. Waiting for CI to pass here: #34958
| .GetNClick(commonlocators.optionpositionL) | ||
| .last() | ||
| .click({ force: true }); | ||
| agHelper.AssertClassExists( |
There was a problem hiding this comment.
@Harshithazemoso Do we need only class assert here? Just want to clarify this case is need from cypress side or unit testing can also help here.
|
hello @rahulbarwal the pr got approved right ,can we merge this if everything looks fine? |
|
Hey @Harshithazemoso , Also, we have conflicts. Can you please resolve these? Once done, let us know, I'll push for it. |
…label-position-has-no-effect-label-wraps-text-unnecessarily
Hello @rahulbarwal I have solved the conflicts |
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range, codebase verification and nitpick comments (2)
app/client/packages/dsl/src/migrate/tests/JsonFormSwitchCheckBoxPositionMigration.test.ts (1)
1-235: Improve the test case for better readability and maintainability.The test case is well-structured, but there are a few areas that can be improved:
The input and output DSLs are quite large and make the test case harder to read and understand. Consider extracting them into separate files or constants to improve readability.
The test case doesn't have any assertions for specific properties that are expected to change after the migration. Add assertions for specific properties to make the test case more robust and maintainable.
Here's an example of how you can improve the test case:
// Extract the input and output DSLs into separate constants const inputDsl: DSLWidget = { // ... }; const outputDsl: DSLWidget = { // ... }; describe("JSON Form Widget Property Migration", () => { it("should migrate JSON Form widget properties correctly", () => { const newDsl = migrateJsonFormWidgetLabelPositonAndAlignment(inputDsl); // Add assertions for specific properties that are expected to change expect(newDsl.children[0].schema.field1.children.switchField.alignWidget).toEqual("LEFT"); expect(newDsl.children[0].schema.field1.children.checkboxField1.labelPosition).toEqual("Right"); expect(newDsl.children[0].schema.field1.children.checkboxField2.alignWidget).toEqual("LEFT"); // Compare the entire output DSL expect(newDsl).toEqual(outputDsl); }); });app/client/packages/dsl/src/migrate/index.ts (1)
609-610: Consider removing the empty lines to keep the code concise.The two empty lines added after the new conditional block are unnecessary. Please remove them to maintain a clean and concise codebase.
Apply this diff to remove the empty lines:
- -
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (5)
- app/client/packages/dsl/src/migrate/index.ts (2 hunks)
- app/client/packages/dsl/src/migrate/migrations/090-migrate-jsonformwidget-labelposition-and-alignment.ts (1 hunks)
- app/client/packages/dsl/src/migrate/tests/DSLMigration.test.ts (2 hunks)
- app/client/packages/dsl/src/migrate/tests/JsonFormSwitchCheckBoxPositionMigration.test.ts (1 hunks)
- app/client/packages/eslint-plugin/package.json (1 hunks)
Files skipped from review due to trivial changes (1)
- app/client/packages/eslint-plugin/package.json
Additional comments not posted (6)
app/client/packages/dsl/src/migrate/migrations/090-migrate-jsonformwidget-labelposition-and-alignment.ts (1)
1-6: Great job with the imports and function signature! 👍The imports are correctly sourced, and the function signature clearly conveys the purpose of the migration function. The naming convention for the function is also appropriate.
app/client/packages/dsl/src/migrate/index.ts (3)
93-93: Excellent work adding the new import statement!The import statement for
migrateJsonFormWidgetLabelPositonAndAlignmentis correctly added.
97-97: Great job incrementing theLATEST_DSL_VERSION!Incrementing
LATEST_DSL_VERSIONfrom 90 to 91 correctly reflects the addition of the new migration function.
605-608: Excellent implementation of the new migration logic!The new conditional block correctly checks for DSL version 90, executes
migrateJsonFormWidgetLabelPositonAndAlignment, and updates the DSL version toLATEST_DSL_VERSION. Well done!app/client/packages/dsl/src/migrate/tests/DSLMigration.test.ts (2)
93-93: Great job adding the new import statement! 👍The import statement for module
m90is correctly added.
934-942: Excellent work adding the new migration entry! 🙌The migration for version 90 is properly registered:
- It correctly references the new
m90module.- The migration function
migrateJsonFormWidgetLabelPositonAndAlignmentis specified.- The
versionis set to 90 which follows the sequence.This will ensure the new migration runs when the
migrateDSLfunction is called.
| return traverseDSLAndMigrate(currentDSL, (widget: WidgetProps) => { | ||
| if (widget.type == "JSON_FORM_WIDGET") { | ||
| const jsonFormWidgetProps = widget; | ||
| Object.keys(jsonFormWidgetProps.schema).forEach((key) => { | ||
| const field = jsonFormWidgetProps.schema[key]; | ||
| if (field.children) { | ||
| Object.keys(field.children).forEach((childKey) => { | ||
| const childField = field.children[childKey]; | ||
|
|
||
| if ( | ||
| childField.fieldType === "Switch" && | ||
| childField.alignWidget === "RIGHT" | ||
| ) { | ||
| childField.alignWidget = "LEFT"; | ||
| } | ||
|
|
||
| if ( | ||
| childField.fieldType === "Checkbox" && | ||
| childField.alignWidget === "LEFT" | ||
| ) { | ||
| field.children[childKey] = { | ||
| ...childField, | ||
| labelPosition: "Right", | ||
| }; | ||
| } | ||
| if ( | ||
| childField.fieldType === "Checkbox" && | ||
| childField.alignWidget === "RIGHT" | ||
| ) { | ||
| childField.alignWidget = "LEFT"; | ||
| field.children[childKey] = { | ||
| ...childField, | ||
| labelPosition: "Left", | ||
| }; | ||
| } | ||
| }); | ||
| } | ||
| }); | ||
| } | ||
| }); |
There was a problem hiding this comment.
The migration logic is correct, but there's room for improvement. 🚀
The function correctly traverses the DSL and applies the necessary migrations for "Switch" and "Checkbox" fields. However, consider the following suggestions to enhance the code:
-
Use a recursive function to traverse the schema instead of nested loops. This will simplify the code and make it more readable.
-
Extract the checks for field types into separate functions. This will improve code organization and make it easier to understand the purpose of each check.
-
Consolidate the migration logic for "Checkbox" fields into a single condition. This will reduce code duplication and make the logic more concise.
Here's an example of how you can refactor the code:
const migrateCheckboxField = (field: any) => {
if (field.alignWidget === "LEFT") {
field.labelPosition = "Right";
} else if (field.alignWidget === "RIGHT") {
field.alignWidget = "LEFT";
field.labelPosition = "Left";
}
};
const migrateSwitchField = (field: any) => {
if (field.alignWidget === "RIGHT") {
field.alignWidget = "LEFT";
}
};
const migrateField = (field: any) => {
if (field.fieldType === "Switch") {
migrateSwitchField(field);
} else if (field.fieldType === "Checkbox") {
migrateCheckboxField(field);
}
if (field.children) {
Object.values(field.children).forEach(migrateField);
}
};
export const migrateJsonFormWidgetLabelPositonAndAlignment = (
currentDSL: DSLWidget,
) => {
return traverseDSLAndMigrate(currentDSL, (widget: WidgetProps) => {
if (widget.type === "JSON_FORM_WIDGET") {
Object.values(widget.schema).forEach(migrateField);
}
});
};These changes will make the code more maintainable and easier to understand. Let me know if you have any further questions!
|
@Harshithazemoso CI is failing for this PR: https://github.com/appsmithorg/appsmith/actions/runs/11047220760/job/30688045582?pr=34958 please review and fix. |
5 participants
…s text unnecessarily #33793
Description
Tip
Add a TL;DR when the description is longer than 500 words or extremely technical (helps the content, marketing, and DevRel team).
Please also include relevant motivation and context. List any dependencies that are required for this change. Add links to Notion, Figma or any other documents that might be relevant to the PR.
Fixes #
33793or
Fixes
https://github.com/appsmithorg/appsmith/issues/33793Warning
If no issue exists, please create an issue first, and check with the maintainers if the issue is valid.
Automation
/ok-to-test tags="@tag.All"
🔍 Cypress test results
Caution
If you modify the content in this section, you are likely to disrupt the CI result for your PR.
Communication
Should the DevRel and Marketing teams inform users about this change?
SCREENSHOTS:
VIDEO:
Video for cypress and fixing the bug
Summary by CodeRabbit
New Features
Bug Fixes