-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
fix(nc-gui): single select overflow fix #10050
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
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe changes in the Changes
Suggested labels
Suggested reviewers
Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 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
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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/nc-gui/components/cell/SingleSelect.vue(1 hunks)
🔇 Additional comments (3)
packages/nc-gui/components/cell/SingleSelect.vue (3)
Line range hint 452-461: LGTM! Well-structured option creation UI
The implementation provides excellent user feedback with:
- Clear visual hierarchy using flex layout
- Proper internationalization
- Intuitive icon usage
Line range hint 474-524: LGTM! Well-organized styles with proper scoping
The styling implementation follows Vue.js best practices:
- Proper use of scoped styles
- Appropriate usage of deep selectors for third-party component styling
- Consistent use of Tailwind utilities
390-390: Verify similar select components for consistent width handling
Let's check if other select components might benefit from the same width fix.
✅ Verification successful
Let me check the actual implementation details of these components to make a more informed decision.
Fixed width implementation is specific to this component's use case
The width implementation (!w-[360px]) in SingleSelect.vue is specific to its usage context and doesn't indicate a pattern that needs to be replicated. Other components with fixed widths (!w-[NNNpx]) are using different values appropriate to their contexts:
- UITypesOptionsWithSearch.vue uses
!w-[164px]for an image - LinkedItems.vue uses
!w-[158px]for a placeholder image - SingleSelect.vue uses
!w-[360px]for the select dropdown
The implementation is correctly addressing the overflow issue specific to this component without affecting other select components in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other select components with potential width issues
echo "Searching for other select components with width classes..."
rg -l "class=\".*w-full.*\".*<(NcSelect|a-select)" packages/nc-gui/components/
echo "Searching for other components with fixed width implementations..."
rg -l "class=\".*!w-\[\d+px\].*\"" packages/nc-gui/components/
Length of output: 1088
Script:
#!/bin/bash
# Check the actual width implementations in the found files
echo "Checking width implementations in select components:"
rg -A 2 -B 2 "!w-\[\d+px\]" packages/nc-gui/components/cell/SingleSelect.vue packages/nc-gui/components/virtual-cell/components/LinkedItems.vue packages/nc-gui/components/smartsheet/column/UITypesOptionsWithSearch.vue
echo -e "\nChecking select component implementations:"
rg -A 2 -B 2 "<(NcSelect|a-select)" packages/nc-gui/components/cell/SingleSelect.vue packages/nc-gui/components/virtual-cell/components/LinkedItems.vue packages/nc-gui/components/smartsheet/column/UITypesOptionsWithSearch.vue
Length of output: 3414
| ref="aselect" | ||
| v-model:value="vModel" | ||
| class="w-full overflow-hidden" | ||
| class="!w-[360px] overflow-hidden" |
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.
🛠️ Refactor suggestion
Consider using a responsive width approach
The fixed width of !w-[360px] with an important modifier might cause issues:
- It could overflow on smaller screens
- The use of
!importantsuggests potential CSS specificity issues
Consider using a more flexible width approach:
-class="!w-[360px] overflow-hidden"
+class="w-full max-w-[360px] overflow-hidden"This maintains the desired maximum width while being responsive on smaller screens.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| class="!w-[360px] overflow-hidden" | |
| class="w-full max-w-[360px] overflow-hidden" |
|
Uffizzi Preview |
7cef5ae to
1835498
Compare
1835498 to
fa9a324
Compare
Change Summary
Change type
style: (formatting, missing semi colons, etc; no production code change)\