fix:icon align when datatype is of image #35992
Conversation
WalkthroughThe changes introduce a new property, Changes
Suggested labels
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
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 (1)
- app/client/src/widgets/BaseInputWidget/component/index.tsx (2 hunks)
Additional comments not posted (3)
app/client/src/widgets/BaseInputWidget/component/index.tsx (3)
526-526: Good practice: Use of constants.Defining
ICON_ALIGN_RIGHTas a constant improves code readability and maintainability by avoiding magic strings.
527-531: Well-implemented: Conditional rendering.The conditional rendering of
rightElementensures that the icon is only displayed wheniconAlignis 'right' andiconNameis provided. This is a good approach to avoid unnecessary rendering.
567-567: Correct integration: Usage ofrightElement.The
rightElementis correctly passed as a prop to theStyledNumericInputcomponent, ensuring proper integration with the existing rendering logic.
|
This PR has not seen activitiy for a while. It will be closed in 7 days unless further activity is detected. |
|
@appsmithorg/query-js-pod , Please review this pr if you have enough bandwidth. |
|
@saiprabhu-dandanayak thank you for your contribution 🙏 This issue doesn't come under the purview of QueryJS Pod, tagging @rahulbarwal to take this forward! |
|
This PR has not seen activitiy for a while. It will be closed in 7 days unless further activity is detected. |
|
Hey @saiprabhu-dandanayak, Sorry for the delay on this PR. Run |
|
Hi @rahulbarwal , fixed liniting issue and updated the pr , could you pls check this. |
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
app/client/src/widgets/BaseInputWidget/component/index.tsx (1)
Line range hint
1-1024: Class, let's review some areas for improvement in our code!
TODO comments need attention
There's a TODO comment on line 26 that needs to be addressed. Remember, TODO comments should be temporary and resolved as soon as possible.Would you like me to create a GitHub issue to track this TODO item?
Consider using TypeScript strict mode
The code could benefit from stricter type checking. Consider enabling TypeScript's strict mode to catch more potential errors at compile-time.Improve error handling
The error handling in the component could be more robust. Consider adding more specific error messages and handling edge cases.Optimize performance
The component has many conditional renders and complex logic. Consider memoizing some of the functions or using React.memo to optimize performance.Enhance accessibility
Ensure that all input elements have proper aria labels and roles for better accessibility.Unit tests
I don't see any unit tests in this file. It's crucial to have comprehensive unit tests for such a complex component.Would you like me to help create a set of unit tests for this component?
Remember, class, these suggestions are meant to improve our code quality and make our application more robust and maintainable. Keep up the good work!
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- app/client/src/widgets/BaseInputWidget/component/index.tsx (1 hunks)
Additional comments not posted (1)
app/client/src/widgets/BaseInputWidget/component/index.tsx (1)
560-560: Class, pay attention to this important addition!Well done on adding the
rightElementprop to theStyledNumericInputcomponent. This change appears to address the icon alignment issue mentioned in the pull request objectives. However, let's make sure we understand the full impact of this change.To ensure this change doesn't affect other parts of the codebase, let's run a quick check:
This will help us verify if there are any other places where we might need to make similar changes.
Verification successful
Let's Correct Our Search Approach!
It looks like the previous search attempt encountered an issue recognizing the
typescriptfile type. To ensure we accurately locate instances ofStyledNumericInputand thegetRightIconmethod, we'll adjust our search parameters accordingly.
Excellent Attention to Detail!
After reviewing the search results, it's clear that the addition of the
rightElementprop usingthis.getRightIcon()is confined solely to theBaseInputWidgetcomponent. There are no other instances whereStyledNumericInpututilizes thegetRightIconmethod in different parts of the codebase. This ensures that the change is localized and won't inadvertently affect other components or functionalities.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for other occurrences of StyledNumericInput and its usage # Search for StyledNumericInput usage echo "Searching for StyledNumericInput usage:" rg --type typescript "StyledNumericInput" -A 5 -B 5 # Search for getRightIcon method echo "Searching for getRightIcon method:" rg --type typescript "getRightIcon" -A 5 -B 5Length of output: 335
Script:
#!/bin/bash # Description: Corrected search for StyledNumericInput and getRightIcon in TypeScript files # Search for StyledNumericInput usage in .ts and .tsx files echo "Searching for StyledNumericInput usage:" rg "StyledNumericInput" -A 5 -B 5 --glob "*.ts" --glob "*.tsx" # Search for getRightIcon method in .ts and .tsx files echo "Searching for getRightIcon method:" rg "getRightIcon" -A 5 -B 5 --glob "*.ts" --glob "*.tsx"Length of output: 7023
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (2)
app/client/src/widgets/BaseInputWidget/component/index.test.tsx (2)
8-32: Excellent helper function, but let's add a little extra credit!Your
renderBaseInputComponentfunction is like a well-organized desk, ready for any test to sit down and work. However, to make it even better, consider adding a comment explaining its purpose and usage. It's like labeling your desk drawers – it helps others understand quickly!Here's a suggestion:
/** * Helper function to render BaseInputComponent with default props. * @param props - Optional props to override defaults. * @returns Rendered component with testing utilities. */ const renderBaseInputComponent = ( props: Partial<BaseInputComponentProps> = {}, ) => { // ... (rest of the function) };This way, future students (I mean, developers) will know exactly how to use this helpful tool!
34-52: A good start on your test, but let's aim for an A+!Your test case is like a well-written essay, focusing on an important topic. However, to make it truly outstanding, let's add a few more details:
Test description: Make it more specific, like "Icon should be visible and aligned to the right when the input type is NUMBER and iconAlign is set to 'right'".
Alignment check: Add an assertion to verify the icon's alignment. You can check if the icon's parent element has a class or style indicating right alignment.
Negative test: Consider adding a test case where the icon should not be visible (e.g., when iconName is not provided).
Here's an example of how you might improve the alignment check:
const iconWrapper = container.querySelector('.input-icon-wrapper'); expect(iconWrapper).toHaveStyle('justify-content: flex-end');Remember, in testing, we want to be thorough, just like double-checking your homework!
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- app/client/src/widgets/BaseInputWidget/component/index.test.tsx (1 hunks)
Additional comments not posted (2)
app/client/src/widgets/BaseInputWidget/component/index.test.tsx (2)
1-6: Well done on your imports, class!You've done a splendid job importing all the necessary modules for our test. It's like you've packed your backpack with all the right tools for a successful learning journey!
1-52: Overall, a commendable effort on your test file!Class, this test file is like a well-written essay. It has a clear introduction (imports), a strong body (helper function and test case), and addresses the main point of our lesson (icon visibility issue).
However, remember that in the pursuit of knowledge, there's always room for improvement. Let's work on adding more detailed comments, expanding our test coverage, and making our assertions more precise.
Keep up the good work, and don't forget to apply these lessons to your future coding assignments!
|
@saiprabhu-dandanayak client-lint is failing for latest changes: https://github.com/appsmithorg/appsmith/actions/runs/11032799313/job/30642535328?pr=36473#step:9:4550 Please fix and run |
Description
Bug Link
I have raised this PR In-order to fix
alignmentof theiconininput componentwhendatatypeis selected asnumberand position is selected asrightscreenshot
Before issue is resolved , icon dissapears
After the issue is resolved
Cypress Testing
unit Testcases
Cypress Testing video
Inputv2_Icon_Align_Spec.js.mp4
Summary by CodeRabbit
New Features
rightElementprop in theBaseInputComponent, allowing users to display an icon on the right side of the input field based on specified properties.Tests
BaseInputComponentto validate the rendering and behavior of the component, including checks for icon visibility and alignment.