chore:remove space b/w form and CTA onboarding page #35985
Conversation
WalkthroughThe recent changes introduce a Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Form
participant RadioButtonControl
participant ReduxForm
User->>Form: Interacts with form
Form->>RadioButtonControl: Renders radio buttons
RadioButtonControl->>ReduxForm: Updates form state on selection
ReduxForm-->>Form: Notifies state change
Form-->>User: Displays updated selection
Assessment against linked issues
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
|
![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
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (6)
- app/client/src/components/formControls/RadioButtonControl.tsx (1 hunks)
- app/client/src/pages/Editor/DataSourceEditor/JSONtoForm.tsx (1 hunks)
- app/client/src/pages/Editor/SaaSEditor/DatasourceForm.tsx (2 hunks)
- app/client/src/utils/formControl/FormControlRegistry.tsx (2 hunks)
- app/client/src/utils/formControl/formControlTypes.ts (1 hunks)
- app/server/appsmith-plugins/googleSheetsPlugin/src/main/resources/form.json (1 hunks)
Files skipped from review due to trivial changes (2)
- app/client/src/pages/Editor/DataSourceEditor/JSONtoForm.tsx
- app/client/src/pages/Editor/SaaSEditor/DatasourceForm.tsx
Additional context used
Biome
app/client/src/components/formControls/RadioButtonControl.tsx
[error] 33-33: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
Additional comments not posted (8)
app/client/src/utils/formControl/formControlTypes.ts (1)
21-21: LGTM!The addition of
RADIO_BUTTONexpands the set of form control types available for use within the application.The code changes are approved.
app/client/src/components/formControls/RadioButtonControl.tsx (4)
1-7: LGTM!The necessary modules and types for the
RadioButtonControlcomponent are correctly imported.The code changes are approved.
9-22: LGTM!The
RadioButtonControlclass correctly extendsBaseControland implements the necessary methods.The code changes are approved.
31-53: LGTM!The
renderComponentfunction correctly handles the rendering and change events for the radio button group.The code changes are approved.
Tools
Biome
[error] 33-33: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
55-59: LGTM!The
RadioButtonControlPropsinterface correctly defines the props for theRadioButtonControlcomponent, and the component is correctly exported.The code changes are approved.
app/server/appsmith-plugins/googleSheetsPlugin/src/main/resources/form.json (1)
36-36: LGTM!The change to the control type for the "Permissions | Scope" configuration property enhances the user experience by making the selection process clearer and more immediate.
The code changes are approved.
app/client/src/utils/formControl/FormControlRegistry.tsx (2)
38-39: LGTM!The new imports for
RadioButtonControlPropsandRadioButtonControlare correctly added.
188-192: LGTM!The new form control builder for
RADIO_BUTTONis correctly implemented and follows the existing pattern.
|
@Shivam-z Should we consider updating an existing test case or adding a new one for unit testing? |
|
@sagar-qa007 we can add a jest unit testing for RadioButtonControl. |
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range, codebase verification and nitpick comments (2)
app/client/src/components/formControls/RadioButtonControl.test.tsx (2)
26-35: Properties for RadioButtonControlThe
radioButtonPropsobject is well-structured and includes all necessary properties to fully configure theRadioButtonControlfor testing. However, theformNameproperty is set as an empty string, which might be intentional but could potentially affect form behavior in some contexts.Consider specifying a non-empty default value for
formNameor ensuring that this property is handled correctly when empty.- formName: "", + formName: "defaultForm",
82-109: Test case: Option selection behaviorThis test case checks the interactive behavior of the
RadioButtonControl, specifically that clicking an option updates the selection as expected. The use ofclickevents and subsequent checks for the checked state of options is a good practice to ensure the component's responsiveness to user inputs.However, the reassignment of
radioButtonPropsin line 83 is unnecessary since you're not modifying any properties. This could be removed to simplify the test setup.- radioButtonProps = { - ...radioButtonProps, - };
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- app/client/src/components/formControls/RadioButtonControl.test.tsx (1 hunks)
Additional comments not posted (6)
app/client/src/components/formControls/RadioButtonControl.test.tsx (6)
1-8: Review of imports and initial setupThe imports and initial setup look appropriate for the testing environment you're working with. Using
react-reduxandredux-formalong withtestUtilsfor rendering components in a test environment is standard practice. Good job on including@testing-library/jest-domfor extended assertions.
12-14: Utility component for testingThe
TestFormcomponent is a simple and effective way to wrap theRadioButtonControlfor testing purposes. This is a neat trick to ensure that the form-related props are correctly passed down. Nicely done!
16-18: Redux Form setupThe use of
reduxFormto create a higher-order componentReduxFormDecoratoris correctly implemented. This setup is crucial for integratingredux-formin your tests, ensuring that form state and behaviors are correctly handled.
20-24: Mock options setupThe array of
mockOptionsis well-defined, providing clear and distinct values for testing theRadioButtonControl. This setup ensures that each option is rendered and behaves as expected during the tests.
42-57: Test case: Rendering and option countThis test case effectively checks that the
RadioButtonControland its options are rendered correctly. UsingwaitForandscreen.getByTestIdto assert the presence of the component andgetAllByRoleto count the options are appropriate methods for these assertions. The test is clear and covers the basic rendering and functionality of the component.
59-80: Test case: Default selected optionThe test case for checking the default selected option is well-implemented. It ensures that the initial state of the radio buttons matches the expected behavior based on the
initialValueproperty. This test is crucial for verifying that the component respects its initial configuration.
|
This PR has not seen activitiy for a while. It will be closed in 7 days unless further activity is detected. |
|
@sagar-qa007 as discussed earlier, I have added a jest unit test case for RadioButtonControl. |
|
Tagging @AmanAgarwal041 @sneha122 for review |
ayushpahwa
requested review from
AmanAgarwal041 and
sneha122
and removed request for
ayushpahwa
|
Hello @Shivam-z, |
NilanshBansal
added
Query & JS Pod
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (3)
- app/client/src/components/formControls/RadioButtonControl.tsx (1 hunks)
- app/client/src/pages/Editor/DataSourceEditor/JSONtoForm.tsx (1 hunks)
- app/client/src/pages/Editor/DataSourceEditor/index.tsx (0 hunks)
Files not reviewed due to no reviewable changes (1)
- app/client/src/pages/Editor/DataSourceEditor/index.tsx
Additional comments not posted (2)
app/client/src/components/formControls/RadioButtonControl.tsx (2)
10-24: Great work on implementing the RadioButtonControl class! 👍The class correctly extends
BaseControland implements the necessary methods to create a radio button selection interface for forms using Redux Form. The control type is appropriately set to "RADIO_BUTTON", and therendermethod properly utilizes theFieldcomponent to link the radio button group to the form state.Keep up the excellent work! 🌟
39-64: Excellent implementation of the renderComponent function! 🎉The function does a great job of handling the rendering of the radio buttons and managing the change events. The
onChangeHandlercorrectly updates the form state when a radio button is selected, ensuring that the selected value is properly synchronized.I appreciate the use of
StyledRadioGroupto style the radio buttons in a vertical layout with appropriate spacing. This enhances the visual presentation and improves the user experience.The function also correctly maps the provided options to render the radio buttons and initializes the selected value based on the
initialValueprop. This ensures that the component behaves as expected.Fantastic work! Keep it up! 🚀
chore/refactored missed colone in FormContainer height property
…mithorg#30523/chore-remove_unnecessary_space_between_form_and_CTA
Shivam-z
force-pushed
the
Task-#30523/chore-remove_unnecessary_space_between_form_and_CTA
branch
from
516970a
to
8dcc55b
Compare
|
@AmanAgarwal041 I have rebased my current branch with the release branch. |
|
@Shivam-z There are still some test failures on the branch, can you please have a look at them ? |
|
@AmanAgarwal041 I don't know why GoogleSheets_spec.ts file is failing in the workflow run. Screencast.from.26-09-24.05.26.59.PM.IST.webm |
|
@Shivam-z It should be fixed now for Google sheet. Other specs are failing though. Can you try others as well on the local ? |
|
@jsartisan could you please review this PR? |
…ading for cypress failures.
|
@AmanAgarwal041 I’ve identified the root cause of the test case failures. The issue stemmed from the removal of the following height property in the DSEditorWrapper: height: calc(100vh - ${(props) => props.theme.headerHeight});This height adjustment caused Cypress to fail in locating the required elements, leading to the test failures. In this commit, I’ve restored the original height value. Could you please trigger the workflow again to verify the fix?` |
|
Hey @Shivam-z , these are the test cases that are failing. Can you please check ? |
…mithorg#30523/chore-remove_unnecessary_space_between_form_and_CTA
|
@AmanAgarwal041 @NilanshBansal , I have fixed GoogleSheet_specs file and rebased current branch with relase with latest changes. |
|
@Shivam-z There seems to be some lint errors. Can you fix those https://github.com/appsmithorg/appsmith/actions/runs/11130343332/job/31021794899?pr=35985 ? |
|
@AmanAgarwal041 I have fixed client linting errors. |
|
Here is one more linting error! @Shivam-z can you take a look at it ? |
|
@AmanAgarwal041 fixed above linting error |
Description
Following are the improvements made in this PR:
dropdown by radio buttons.
Fixes #30523
output screenshot:
Desired design:
*Fixes #
35950or
Fixes
Issue URLWarning
If no issue exists, please create an issue first, and check with the maintainers if the issue is valid.
Automation
/ok-to-test tags=""
🔍 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?
Summary by CodeRabbit
Summary by CodeRabbit
New Features
RadioButtonControlcomponent for improved form control options.FormControlRegistryto support radio button controls.UI Changes
FormContainerfor proper rendering.DatasourceFormfor better clarity.Tests
RadioButtonControlcomponent to ensure proper functionality and user interaction.