-
Notifications
You must be signed in to change notification settings - Fork 3.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: remove-clear-option-for-required-select-widget #35060
feat: remove-clear-option-for-required-select-widget #35060
Conversation
WalkthroughThe recent changes enhance the Changes
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 as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- app/client/src/widgets/SelectWidget/component/SelectButton.test.tsx (1 hunks)
- app/client/src/widgets/SelectWidget/component/SelectButton.tsx (3 hunks)
- app/client/src/widgets/SelectWidget/component/index.tsx (2 hunks)
- app/client/src/widgets/SelectWidget/widget/index.tsx (1 hunks)
Additional comments not posted (7)
app/client/src/widgets/SelectWidget/component/SelectButton.tsx (2)
18-18
: Property Addition ApprovedThe addition of the
isRequired
property to theSelectButtonProps
interface is appropriate and aligns with the PR objectives.
32-32
: Usage ofisRequired
Property ApprovedThe usage of the
isRequired
property in the destructuredprops
and the rendering logic of the button is correct. It ensures that the cancel icon is not displayed whenisRequired
is true.Also applies to: 43-43
app/client/src/widgets/SelectWidget/component/SelectButton.test.tsx (2)
62-68
: Test Case forisRequired
True ApprovedThe test case correctly verifies that the cancel button does not render when
isRequired
is true.
70-76
: Test Case forisRequired
False ApprovedThe test case correctly verifies that the cancel button renders when
isRequired
is false.app/client/src/widgets/SelectWidget/component/index.tsx (2)
459-459
: Property Addition ApprovedThe addition of the
isRequired
property to theSelectComponentProps
interface is appropriate and aligns with the PR objectives.
432-432
: Usage ofisRequired
Property ApprovedThe usage of the
isRequired
property in theSelectButton
component within theSelectComponent
class is correct. It ensures that the property is passed down appropriately.app/client/src/widgets/SelectWidget/widget/index.tsx (1)
852-852
: LGTM! EnsureisRequired
property is correctly utilized.The addition of the
isRequired
property to theSelectComponent
is approved. Ensure that theSelectComponent
properly handles theisRequired
property to control the rendering behavior.
HI @somangshu, Could you review this pr. |
@appsmithorg/contributor-support can we get this reviewed. |
This PR has not seen activitiy for a while. It will be closed in 7 days unless further activity is detected. |
Hi @somangshu, Could you please review this pr. |
@rahulbarwal @jacquesikot can you please review this |
@AnnaHariprasad5123 Client liniting is failing for changes in this PR Logs |
Hi @rahulbarwal, Could you check now. I fixed linting issues and pushed the changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- app/client/src/widgets/SelectWidget/component/SelectButton.test.tsx (1 hunks)
- app/client/src/widgets/SelectWidget/component/SelectButton.tsx (3 hunks)
- app/client/src/widgets/SelectWidget/component/index.tsx (2 hunks)
Additional comments not posted (3)
app/client/src/widgets/SelectWidget/component/SelectButton.tsx (1)
18-18
: LGTM! EnsureisRequired
usage is consistent.The addition of the
isRequired
property and its integration into the rendering logic is well-implemented. The logic correctly prevents the cancel icon from displaying whenisRequired
is true.Verify that the
isRequired
property is used consistently across the codebase to avoid any discrepancies.Also applies to: 28-28, 43-43
app/client/src/widgets/SelectWidget/component/SelectButton.test.tsx (1)
62-68
: Test cases forisRequired
property are well-implemented.The added test cases effectively verify the conditional rendering of the cancel button based on the
isRequired
property. This enhances the robustness of the test suite.Also applies to: 70-76
app/client/src/widgets/SelectWidget/component/index.tsx (1)
432-432
: Enhancements toSelectComponentProps
are well-structured.The introduction of new properties, including
isRequired
, enhances the configurability and functionality of theSelectComponent
. The changes align with the PR objectives and improve user experience.Ensure that the integration of these new properties, particularly
isRequired
, is thoroughly tested and documented across the codebase.Also applies to: 442-480
Verification successful
Integration of
isRequired
and Other Properties VerifiedThe search results indicate that the new properties, including
isRequired
, are integrated throughout the codebase. They appear in various contexts, such as component props, conditional checks, and styling. This suggests that the properties are well-integrated and likely tested.
- Integration Points:
isRequired
is used in prop assignments and conditional logic.- Other properties like
accentColor
,borderRadius
, andboxShadow
are also used extensively in styling and component configurations.For thorough verification, ensure these properties are covered in unit tests and documented in the relevant parts of the codebase.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify integration and documentation of new properties in `SelectComponentProps`. # Test: Search for the usage of new properties in the codebase. Expect: Proper integration and documentation. rg --type tsx 'accentColor|borderRadius|boxShadow|compactMode|hasError|isRequired'Length of output: 112
Script:
#!/bin/bash # Description: Verify integration and documentation of new properties in `SelectComponentProps`. # Find all .tsx files and search for the new properties within them. fd --extension tsx --exec rg 'accentColor|borderRadius|boxShadow|compactMode|hasError|isRequired' {}Length of output: 108273
HI @rahulbarwal, Could you please review this pr. |
@AnnaHariprasad5123 Sorry for the delay. Can you please fix it? |
Hi @rahulbarwal,
I ordered them alphabetically in previous commit but still it is showing error as they are not ordered. Can you conform the order of these props. |
@AnnaHariprasad5123 It is talking about this line. |
@rahulbarwal, Thanks for the info. Could you check now and let me know if any actions required from my end. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- app/client/src/widgets/SelectWidget/component/index.tsx (2 hunks)
Additional comments not posted (3)
app/client/src/widgets/SelectWidget/component/index.tsx (3)
428-428
: Correct usage ofisRequired
prop.The
isRequired
prop is correctly passed to theStyledSingleDropDown
component, aligning with the PR objectives to manage the visibility of the remove button based on this property.
442-480
: New properties inSelectComponentProps
are well-defined.The new properties
accentColor
,borderRadius
,boxShadow
,compactMode
,hasError
, andisRequired
are correctly defined with appropriate types. The rearrangement of properties maintains consistency and readability.
Line range hint
1-480
:
Successful integration of enhancements.The changes effectively enhance the
SelectComponent
by introducing new styling options and improving validation capabilities. These modifications align with the PR objectives, ensuring better integration into forms and an improved user experience.
Hi @rahulbarwal, |
@AnnaHariprasad5123 On cypress CI, some other select related specs are failing. Please checkout description of this PR: #35562 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- app/client/cypress/e2e/Regression/ClientSide/Widgets/ListV2/Childwigets/List_Select_Widgets_spec.js (1 hunks)
- app/client/cypress/e2e/Regression/ClientSide/Widgets/Select/Select2_Spec.ts (1 hunks)
Additional context used
Path-based instructions (2)
app/client/cypress/e2e/Regression/ClientSide/Widgets/ListV2/Childwigets/List_Select_Widgets_spec.js (1)
Pattern
app/client/cypress/**/**.*
:app/client/cypress/e2e/Regression/ClientSide/Widgets/Select/Select2_Spec.ts (1)
Pattern
app/client/cypress/**/**.*
:
Additional comments not posted (2)
app/client/cypress/e2e/Regression/ClientSide/Widgets/ListV2/Childwigets/List_Select_Widgets_spec.js (1)
152-152
: LGTM! Test case correctly verifies the absence of the cancel button.The assertion ensures that the cancel button does not exist when the select widget is marked as required, aligning with the PR objectives.
app/client/cypress/e2e/Regression/ClientSide/Widgets/Select/Select2_Spec.ts (1)
300-300
: LGTM! Test case correctly verifies the absence of the cancel button.The assertion ensures that the cancel button does not exist when the select widget is marked as required, aligning with the PR objectives.
Hi @rahulbarwal, Could you check now. |
Hi @rahulbarwal, check once. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- app/client/cypress/e2e/Regression/ClientSide/Widgets/ListV2/Childwigets/List_Select_Widgets_spec.js (1 hunks)
- app/client/cypress/e2e/Regression/ClientSide/Widgets/Select/Select2_Spec.ts (2 hunks)
- app/client/cypress/locators/commonlocators.json (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- app/client/cypress/e2e/Regression/ClientSide/Widgets/ListV2/Childwigets/List_Select_Widgets_spec.js
- app/client/cypress/e2e/Regression/ClientSide/Widgets/Select/Select2_Spec.ts
Additional context used
Path-based instructions (1)
app/client/cypress/locators/commonlocators.json (1)
Pattern
app/client/cypress/**/**.*
:
Additional comments not posted (1)
app/client/cypress/locators/commonlocators.json (1)
76-76
: Addition ofselectClearButton
locator looks good!The new locator
"selectClearButton": "selectbutton.btn.cancel"
has been added correctly. Ensure that the CSS selector accurately targets the intended element in the application's DOM.
app/client/src/widgets/SelectWidget/component/SelectButton.test.tsx
Outdated
Show resolved
Hide resolved
app/client/src/widgets/SelectWidget/component/SelectButton.test.tsx
Outdated
Show resolved
Hide resolved
HI @rahulbarwal, Could you check now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- app/client/cypress/e2e/Regression/ClientSide/Widgets/Select/Select2_Spec.ts (2 hunks)
- app/client/cypress/locators/commonlocators.json (1 hunks)
- app/client/src/widgets/SelectWidget/component/SelectButton.test.tsx (2 hunks)
Files skipped from review as they are similar to previous changes (3)
- app/client/cypress/e2e/Regression/ClientSide/Widgets/Select/Select2_Spec.ts
- app/client/cypress/locators/commonlocators.json
- app/client/src/widgets/SelectWidget/component/SelectButton.test.tsx
HI @rahulbarwal, I done as you said. Can you check now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- app/client/cypress/e2e/Regression/ClientSide/Widgets/ListV2/Childwigets/List_Select_Widgets_spec.js (2 hunks)
- app/client/cypress/e2e/Regression/ClientSide/Widgets/Select/Select2_Spec.ts (2 hunks)
- app/client/cypress/support/Objects/CommonLocators.ts (1 hunks)
- app/client/src/widgets/SelectWidget/component/SelectButton.test.tsx (3 hunks)
Files skipped from review as they are similar to previous changes (2)
- app/client/cypress/e2e/Regression/ClientSide/Widgets/ListV2/Childwigets/List_Select_Widgets_spec.js
- app/client/src/widgets/SelectWidget/component/SelectButton.test.tsx
Additional context used
Path-based instructions (2)
app/client/cypress/e2e/Regression/ClientSide/Widgets/Select/Select2_Spec.ts (1)
Pattern
app/client/cypress/**/**.*
:app/client/cypress/support/Objects/CommonLocators.ts (1)
Pattern
app/client/cypress/**/**.*
:
Biome
app/client/cypress/e2e/Regression/ClientSide/Widgets/Select/Select2_Spec.ts
[error] 17-17: Shouldn't redeclare 'locators'. Consider to delete it or rename it.
'locators' is defined here:
(lint/suspicious/noRedeclare)
Additional comments not posted (2)
app/client/cypress/e2e/Regression/ClientSide/Widgets/Select/Select2_Spec.ts (1)
304-304
: Simplified test case looks good.The test case has been simplified to check for the absence of the clear button, aligning with the new
isRequired
property functionality.app/client/cypress/support/Objects/CommonLocators.ts (1)
337-338
: Addition of test IDs is appropriate.The new properties
_selectClearButton_testId
and_selectClearButton_dataTestId
enhance the testability of the select widget by providing specific identifiers.
app/client/cypress/e2e/Regression/ClientSide/Widgets/Select/Select2_Spec.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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/Select/Select2_Spec.ts (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- app/client/cypress/e2e/Regression/ClientSide/Widgets/Select/Select2_Spec.ts
app/client/cypress/e2e/Regression/ClientSide/Widgets/Select/Select2_Spec.ts
Outdated
Show resolved
Hide resolved
...ent/cypress/e2e/Regression/ClientSide/Widgets/ListV2/Childwigets/List_Select_Widgets_spec.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- app/client/cypress/e2e/Regression/ClientSide/Widgets/ListV2/Childwigets/List_Select_Widgets_spec.js (1 hunks)
- app/client/cypress/e2e/Regression/ClientSide/Widgets/Select/Select2_Spec.ts (1 hunks)
Files skipped from review due to trivial changes (1)
- app/client/cypress/e2e/Regression/ClientSide/Widgets/ListV2/Childwigets/List_Select_Widgets_spec.js
Files skipped from review as they are similar to previous changes (1)
- app/client/cypress/e2e/Regression/ClientSide/Widgets/Select/Select2_Spec.ts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Thanks for your effort and your patience :) @AnnaHariprasad5123
Waiting for CI to pass here: #35562
@sagar-qa007 Please review cypress changes here. |
@AnnaHariprasad5123 we have some failures here: #35562 Please check if they are related to your changes(check description). |
Hi @rahulbarwal, Good morning 1. cypress/e2e/Regression/ClientSide/OtherUIFeatures/GlobalSearch_spec.js : It is failed while setuping the postgres datasource with "host.docker.internal" host address. These things are not related to select widget. |
Seems unrelated, thanks for taking a look. I've triggered another run, I'll monitor it. |
Hi @rahulbarwal, If everthing is ok, Can you merge this pr. |
Hi @rohan-arthur, @somangshu , @contributor-support
Fixes #34262
What's in this pr :
Screenshots :
Please let me know if any changes are required.
Thank you.
Summary by CodeRabbit
Summary by CodeRabbit
New Features
isRequired
property to theSelectButton
,SelectComponent
, andSelectWidget
, allowing users to indicate whether a selection is mandatory.SelectButton
to conditionally display the cancel button based on theisRequired
state.SelectComponent
for better integration into forms.Tests
SelectButton
to ensure correct rendering behavior of the cancel button based on theisRequired
prop.