fix:JSONForm > Switch Field: label position has no effect, label wrap…

[Harshithazemoso]

…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 #33793
or
Fixes https://github.com/appsmithorg/appsmith/issues/33793

[!WARNING]
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"

:mag: 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?

• [ ] Yes
• [ ] No

SCREENSHOTS:

VIDEO: Video for cypress and fixing the bug

Summary by CodeRabbit

• New Features

  • Introduced test cases for the migration of JSON Form widget properties, ensuring correct label positioning and alignment.
  • Enhanced JSON form widget configuration with new label positioning options for fields.
  • Added dynamic label positioning support in CheckboxField and SwitchField components.
  • Updated migration process for JSON Form Widgets to align with new design specifications.
  • Expanded widget locators to improve interaction with switch components in the user interface.

• Bug Fixes

  • Improved locators for various input fields and checkboxes to enhance test automation reliability.

[coderabbitai[bot]]

Walkthrough

This 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

File Path	Change Summary
app/client/cypress/e2e/Regression/ClientSide/Widgets/JSONForm/JSONForm_EditSwitchFied_spec.ts	Added a test specification for validating the conversion of text input fields to switch fields.
app/client/cypress/locators/commonlocators.json	Introduced new locators for various input fields and checkboxes to enhance UI interactions during tests.
app/client/src/widgets/JSONFormWidget/component/Field.tsx	Added labelPosition prop to improve layout configurability, simplifying class name handling.
app/client/src/widgets/JSONFormWidget/component/FieldLabel.tsx	Updated props to include labelPosition, enabling flexible label positioning within the widget.
app/client/src/widgets/JSONFormWidget/fields/CheckboxField.tsx	Introduced labelPosition prop for dynamic label placement based on schema configuration.
app/client/src/widgets/JSONFormWidget/fields/SwitchField.tsx	Added labelPosition prop to allow dynamic label positioning for the switch field based on schema.
app/client/packages/dsl/src/migrate/index.ts	Updated LATEST_DSL_VERSION to 91 and integrated new migration logic for label positions and alignments.
app/client/packages/dsl/src/migrate/migrations/089-migrate-jsonformwidget-labelposition-and-alignment.ts	Introduced a migration function to adjust label positions and alignments for JSON Form Widgets.
app/client/packages/dsl/src/migrate/migrations/090-migrate-jsonformwidget-labelposition-and-alignment.ts	Added a migration function to modify alignment and label positions of child fields within JSON Form Widgets.
app/client/packages/dsl/src/migrate/tests/JsonFormSwitchCheckBoxPositionMigration.test.ts	Created unit tests for the migration function, verifying the correctness of alignment and label position changes.
app/client/cypress/locators/Widgets.json	Added new properties for switch components to enhance functionality regarding alignment and labeling.

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

In the realm of forms, we twist and we turn,
With switches that toggle, and lessons to learn.
Labels now dance from left to right,
Our widgets harmonize, oh what a sight!
With tests to ensure all functions align,
A seamless adventure, our victory divine! ✨

---

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?

Share

• X
• Mastodon
• Reddit
• LinkedIn

Tips

Chat

There are 3 ways to chat with CodeRabbit:

• Review comments: Directly reply to a review comment made by CodeRabbit. Example:
  • I pushed a fix in commit <commit_id>.
  • Generate unit testing code for this file.
  • Open a follow-up GitHub issue for this discussion.
• Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
  • @coderabbitai generate unit testing code for this file.
  • @coderabbitai modularize this function.
• PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
  • @coderabbitai generate interesting stats about this repository and render them as a table.
  • @coderabbitai show all the console.log statements in this repository.
  • @coderabbitai read src/utils.ts and generate unit testing code.
  • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
  • @coderabbitai help me debug CodeRabbit configuration file.

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)

• @coderabbitai pause to pause the reviews on a PR.
• @coderabbitai resume to resume the paused reviews.
• @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
• @coderabbitai full review to do a full review from scratch and review all the files again.
• @coderabbitai summary to regenerate the summary of the PR.
• @coderabbitai resolve resolve all the CodeRabbit review comments.
• @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
• @coderabbitai help to get help.

Other keywords and placeholders

• Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
• Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
• Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

• Visit our Documentation for detailed information on how to use CodeRabbit.
• Join our Discord Community to get help, request features, and share feedback.
• Follow us on X/Twitter for updates and announcements.

Early access features: disabled

We are currently testing the following features in early access:

• Anthropic claude-3-5-sonnet for code reviews: Anthropic claims that the new Claude model has stronger code understanding and code generation capabilities than their previous models. Note: Our default code review model was also updated late last week. Please compare the quality of the reviews between the two models by toggling the early access feature.

Note:

• You can enable or disable early access features from the CodeRabbit UI or by updating the CodeRabbit configuration file.
• Please join our Discord Community to provide feedback and report issues on the discussion post.

[Harshithazemoso]

hello @ApekshaBhosale ,I have made the changes requested ,could you please review the pr

[github-actions[bot]]

This PR has not seen activitiy for a while. It will be closed in 7 days unless further activity is detected.

[Harshithazemoso]

Hello @Nikhil-Nandagopal ,can you assign someone to review the pr ,It has been more than 3 weeks ,its still in review Thank you

[Harshithazemoso]

hello @sagar-qa007 made the changes as suggested could you please review the pr

[sagar-qa007]

@Harshithazemoso can we run test case in cypress ?

[Harshithazemoso]

@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 ?

[rahulbarwal]

@Harshithazemoso can you please resolve merge conflicts here?

[Harshithazemoso]

@Harshithazemoso can you please resolve merge conflicts here?

@rahulbarwal I have resolved them

[Harshithazemoso]

Found some functional issues with these changes. Please check out this video and let me know if you have any more questions.

hello @rahulbarwal made changes can you please check now

[rahulbarwal]

Hey @Harshithazemoso, We would like to suggest some product level changes to the solution you've proposed.

• We should not be renaming the property names in config, ref.
  • Why? All the existing apps, that rely on this property will behave incorrectly after this.
  • How to test? On app.appsmith.com create a JSONform with switch and put position as right. Even though it will not work, but if you export the json file and then import in to your branch changes, observe the behavior.
  • this is something we want to avoid.
• The behavior you have implemented for switch inside JSONForm is different from a "normal" switch widget.
  • How? Normal Switch has both Alignment and Position.
  • We expect similar solution
  • Mandatory: Respect the default alignment, which is on the left, and change the position accordingly (see image).
  • Ideal: Add the alignment property and make it work exactly like the switch widget.

Thank you your effort and contribution and Feel free to let me know if you have any questions. cc: @carinanfonseca , @ashit-rath

[Harshithazemoso]

Hey @Harshithazemoso, We would like to suggest some product level changes to the solution you've proposed.

• We should not be renaming the property names in config, ref.

  • Why? All the existing apps, that rely on this property will behave incorrectly after this.
  • How to test? On app.appsmith.com create a JSONform with switch and put position as right. Even though it will not work, but if you export the json file and then import in to your branch changes, observe the behavior.
  • this is something we want to avoid.

• The behavior you have implemented for switch inside JSONForm is different from a "normal" switch widget.

  • How? Normal Switch has both Alignment and Position.
  • We expect similar solution
  • Mandatory: Respect the default alignment, which is on the left, and change the position accordingly (see image).
  • Ideal: Add the alignment property and make it work exactly like the switch widget.

Thank you your effort and contribution and Feel free to let me know if you have any questions. cc: @carinanfonseca , @ashit-rath

hello @rahulbarwal, I have tried the ideal approach which you mentioned rahul but when I try add "alignment and label position" properties in the switch.ts this properties are being added to other widget property fields also ,so I thought move with the mandatory way where the alignment would be fixed and works similar to checkbox ,is it okay rahul ? can you please confirm this

[Harshithazemoso]

@rahulbarwal The switch widget is like this after making the changes ,so should I proceed like this or does it need any changes ,can you please look into this?

[rahulbarwal]

I have tried the ideal approach which you mentioned rahul but when I try add "alignment and label position" properties in the switch.ts this properties are being added to other widget property fields also ,so I thought move with the mandatory way where the alignment would be fixed and works similar to checkbox ,is it okay rahul ?

These were inputs from our team lets try to follow these:

• Mandatory: Respect the default alignment, which is on the left, and change the position accordingly.
• Ideal: Add the alignment property and make it work exactly like the switch widget.

The switch widget is like this after making the changes ,so should I proceed like this or does it need any changes ,can you please look into this?

yes, just make sure padding is similar to normal widgets when alignment is changed.

[Harshithazemoso]

I have tried the ideal approach which you mentioned rahul but when I try add "alignment and label position" properties in the switch.ts this properties are being added to other widget property fields also ,so I thought move with the mandatory way where the alignment would be fixed and works similar to checkbox ,is it okay rahul ?

These were inputs from our team lets try to follow these:

• Mandatory: Respect the default alignment, which is on the left, and change the position accordingly.
• Ideal: Add the alignment property and make it work exactly like the switch widget.

The switch widget is like this after making the changes ,so should I proceed like this or does it need any changes ,can you please look into this?

yes, just make sure padding is similar to normal widgets when alignment is changed.

hello @rahulbarwal made changes and now the switch widget alignment would be as shown below,now the alignment would be fixed and the position would change accordingly same as checkbox ,but I have the padding which was misisng for both widgets when the position is right ,hope this is what expected . Thank you

[rahulbarwal]

/build-deploy-preview skip-tests=true

[github-actions[bot]]

Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/10037742463. Workflow: On demand build Docker image and deploy preview. skip-tests: true. env: ``. PR: 34298. recreate: .

[ApekshaBhosale]

@rahulbarwal does new spec even testing the intended behaviour? looks like this is an UI issue. cc @sagar-qa007

[github-actions[bot]]

Deploy-Preview-URL: https://ce-34298.dp.appsmith.com

[rahulbarwal]

@Harshithazemoso Behavior is still not there yet. Shared some feedback through this video: https://www.loom.com/share/81524b33b69f4d3897d4069fcb78fde2?sid=ad549576-68f6-4223-a90f-74b906b1660f

• The behavior of checkbox and switch inside jsonForm should be same as the ones outside(i.e. normal widget)
• Secondly the Position property should work on label and not the toggle.

[rahulbarwal]

Also please add proper tests testing this functionality as asked by Apeksha above.

[Harshithazemoso]

@Harshithazemoso Behavior is still not there yet. Shared some feedback through this video: https://www.loom.com/share/81524b33b69f4d3897d4069fcb78fde2?sid=ad549576-68f6-4223-a90f-74b906b1660f

• The behavior of checkbox and switch inside jsonForm should be same as the ones outside(i.e. normal widget)
• Secondly the Position property should work on label and not the toggle.

hello @rahulbarwal you said we shouldnot change the propertyName of alignWidget right ?

[Harshithazemoso]

Also please add proper tests testing this functionality as asked by Apeksha above.

@rahulbarwal in the cypress testing right ,I will add that based on the new changes. Thank you

[rahulbarwal]

hello @rahulbarwal you said we shouldnot change the propertyName of alignWidget right ?

Yes, thats correct, so thats what we have to balance. We cannot update the propertyName because it will break existing apps on prod. So thats why we have shared ideal expectation i.e. to have both alignment and position for labels inside the JSON Form -- exactly like we have for normal widgets(switch/checkbox).

• If for some reason, it still breaks existing apps, we will work with you to come up with a migration plan to ensure everything stays smooth.

in the cypress testing right ,I will add that based on the new changes.

Sounds good. Point to note: we are not adamant on cypress tests, if you think unit tests suffice for the change; add them and the team will review it.

[rahulbarwal]

That is because you are missing hidden check, if you check the release code, this line checks if the current widget is checkbox then only show it otherwise don't.

You have to add this in both labelPosition and alignWidget.

[Harshithazemoso]

hello @rahulbarwal you said we shouldnot change the propertyName of alignWidget right ?

Yes, thats correct, so thats what we have to balance. We cannot update the propertyName because it will break existing apps on prod. So thats why we have shared ideal expectation i.e. to have both alignment and position for labels inside the JSON Form -- exactly like we have for normal widgets(switch/checkbox).

• If for some reason, it still breaks existing apps, we will work with you to come up with a migration plan to ensure everything stays smooth.

in the cypress testing right ,I will add that based on the new changes.

Sounds good. Point to note: we are not adamant on cypress tests, if you think unit tests suffice for the change; add them and the team will review it.

@rahulbarwal the problem is the alignWidget name here in the JsonForm its given for the position field right ,so if we want to add the alignment property then we should definitely change the propertyNames right ?

alignWdget propertyName is already having the functionality of changing the position ,which should be labelPosition by default .

SO now if we are adding both properties I am thinking to add them like this as shown in below image(same as default switch field) ,Unlike earlier ,here the alignWidget would represent the alignment of label and labelPosition would represent the position of label which are correct properties ,is that okay rahul ,can you please confirm this ?

[Harshithazemoso]

hello @rahulbarwal you said we shouldnot change the propertyName of alignWidget right ?

Yes, thats correct, so thats what we have to balance. We cannot update the propertyName because it will break existing apps on prod. So thats why we have shared ideal expectation i.e. to have both alignment and position for labels inside the JSON Form -- exactly like we have for normal widgets(switch/checkbox).

• If for some reason, it still breaks existing apps, we will work with you to come up with a migration plan to ensure everything stays smooth.

in the cypress testing right ,I will add that based on the new changes.

Sounds good. Point to note: we are not adamant on cypress tests, if you think unit tests suffice for the change; add them and the team will review it.

@rahulbarwal the problem is the alignWidget name here in the JsonForm its given for the position field right ,so if we want to add the alignment property then we should definitely change the propertyNames right ?

alignWdget propertyName is already having the functionality of changing the position ,which should be labelPosition by default .

SO now if we are adding both properties I am thinking to add them like this as shown in below image(same as default switch field) ,Unlike earlier ,here the alignWidget would represent the alignment of label and labelPosition would represent the position of label which are correct properties ,is that okay rahul ,can you please confirm this ?

@rahulbarwal shall I proceed?

[rahulbarwal]

omg Sorry forgot to reply, yes please proceed. If we face any hurdle with this approach then we the team will assist you.

[Harshithazemoso]

omg Sorry forgot to reply, yes please proceed. If we face any hurdle with this approach then we the team will assist you.

thank you rahul

[Harshithazemoso]

@rahulbarwal can you pleasecheck and confirm whether updated changes reaches expectations?