Fix #341 : Update CustomTextField for full-width support #353
Conversation
WalkthroughThe Changes
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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: 1
🧹 Outside diff range and nitpick comments (1)
frontend/src/components/TextFieldComponents/CustomTextField/CustomTextField.jsx (1)
122-122: Props validation's got the right flow! 🎤The PropTypes validation is correct, but we could improve the organization.
Consider grouping related props together. Move
fullWidthnext to other layout-related props likeTextFieldWidth:TextFieldWidth: PropTypes.string, + fullWidth: PropTypes.bool, inputHeight: PropTypes.string, id: PropTypes.string, name: PropTypes.string, checkCircleIconVisible: PropTypes.bool, displayCheckCircleIcon: PropTypes.bool, textFieldMargin: PropTypes.string, type: PropTypes.string, - fullWidth: PropTypes.bool, required: PropTypes.bool,
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
frontend/src/components/TextFieldComponents/CustomTextField/CustomTextField.jsx(3 hunks)
🔇 Additional comments (2)
frontend/src/components/TextFieldComponents/CustomTextField/CustomTextField.jsx (2)
33-33: Yo, this prop declaration is straight fire! 🔥
The fullWidth prop follows Material-UI conventions perfectly, with the correct default value and naming pattern.
33-33: Drop the beat while we verify these changes! 🎧
Let's make sure existing components using CustomTextField won't break with this new prop.
Also applies to: 60-61
✅ Verification successful
Yo, this change is safe to drop! 🎤
The default fullWidth=false won't break existing components because:
- Most components explicitly set
TextFieldWidthprop (like "241px" or "100%") - Components that need full width already specify it through
TextFieldWidth="100%" - No components directly rely on the
fullWidthprop in their implementations
The change maintains backward compatibility while providing better control over the field's width through either fullWidth or TextFieldWidth props.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for existing CustomTextField usages to verify impact
echo "Checking CustomTextField usages..."
rg -l "CustomTextField" --type jsx
# Look specifically for TextFieldWidth prop usage
echo "Checking TextFieldWidth prop usage..."
rg "TextFieldWidth=" --type jsx -A 2 -B 2
Length of output: 296
Script:
#!/bin/bash
# Search for existing CustomTextField usages without file type restriction
echo "Checking CustomTextField usages..."
rg -l "CustomTextField"
# Look specifically for fullWidth prop usage
echo "Checking fullWidth prop usage..."
rg "fullWidth" -A 2 -B 2
# Check the actual CustomTextField component definition
echo "Checking CustomTextField component definition..."
rg "CustomTextField" -A 10 -B 10 "frontend/src/components/TextFieldComponents/CustomTextField/CustomTextField.jsx"
Length of output: 4593
Script:
#!/bin/bash
# Let's check how CustomTextField is actually used in the components
echo "Checking actual usage patterns..."
for file in $(rg -l "CustomTextField" | grep -v "CustomTextField.jsx" | grep -v "test" | grep -v "ReadMe.md"); do
echo "=== Checking $file ==="
rg -A 2 -B 2 "<CustomTextField" "$file"
done
Length of output: 12151
frontend/src/components/TextFieldComponents/CustomTextField/CustomTextField.jsx
Outdated
Show resolved
Hide resolved
|
@erenfn Could you elaborate on the issue ? I understand your point about full width, but what other concerns are you referring to related to the full-width aspect ?
|
|
I know. The thing is that we wrap the text field in a div, and I believe that is where this problem occurs. Also, you can clearly see from the second image that it doesn't take up 100% of the space of the parent. |
|
@saksham-malhotra-27 Any updates? |
|
Hey @erenfn ! I got a bit busy, but I came across this: Problem:In the first CustomTextField, I explicitly applied fullWidth as true. However, in the second one, I didn’t: As a result, the views turned out like this: This happens because the default value of fullWidth is set to false. Even if TextFieldWidth is set to 'full', the fullWidth prop still falls back to its default value: Solution:Here are two possible approaches to resolve this:
My Recommendation: Let me know your thoughts on this! Once you approve, I’ll implement the changes and commit them. Apologies again for the delay—I’ll ensure this gets finished within a day. 😊 |
|
Hey! To implement the changes quickly and effectively, I went ahead with the second approach by doing the following: Now, the decision for whether the text field should occupy full width is based on this computed full width property. I also included "stretch" to account for scenarios where terms commonly associated with flexible layouts, like "stretch", might be used. Result: |
|
Thanks, the second approach makes sense. Could you also check other parts (Like |
|
Hey @erenfn! I've checked everything, and it all seems to be working fine on my end. Many of the components use predefined widths like 241px, so it doesn't make much of a difference. Additionally, when fullWidth is set to true, it works as expected. Could you please review the code once? |
There was a problem hiding this comment.
- This is when I set fullWidth=true for the second text field
- This is when I set the width of the parent div of the TextField 100% for the second text field:
You also have to set the width of the outer div element. The screenshots are taken from /popup/create page by zooming out
|
Alright ! working on it, |
|
It should be set to 100% when fullWidth=true |
|
Alright! Will see the issue today. |
|
Hey @erenfn ! Approach 1
However, there are some limitations to consider as it's similar to being hard coded. The "Banner Create", "Popup Create", and "CodeTab" under Settings all need similar adjustments. Although we currently use a fixed width for all three. Approach 2
According to me the 2nd approach is reliable and better than the first one . Let me know your thoughts, I'll change the code accordingly. |
frontend/src/components/TextFieldComponents/CustomTextField/CustomTextField.jsx
Outdated
Show resolved
Hide resolved
|
Hey @erenfn ! I have pushed the changes. |
Issue:
The CustomTextField component does not handle width properly. Specifically, it lacks support for stretching to 100% width.
#341
Implementation: