-
Notifications
You must be signed in to change notification settings - Fork 266
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
fix(input): [input] fix input container 1px higher than input #2504
Conversation
WalkthroughThe changes in this pull request involve updates to the Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
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: 0
🧹 Outside diff range and nitpick comments (2)
packages/theme/src/input/index.less (2)
Line range hint
269-273
: Consider using a distinct font size for mini variant.Currently, both small and mini variants use
var(--tv-Input-small-font-size)
. For better visual hierarchy, consider:
- Creating a separate variable like
--tv-Input-mini-font-size
- Using a slightly smaller value for the mini variant
&-mini { .input-size(var(--tv-Input-mini-height)); - font-size: var(--tv-Input-small-font-size); + font-size: var(--tv-Input-mini-font-size); .@{input-prefix-cls}__inner { - font-size: var(--tv-Input-small-font-size); + font-size: var(--tv-Input-mini-font-size); }
The 1px height difference is likely due to border calculation
The analysis reveals the root cause of the height misalignment:
- The input container has
border: 1px solid var(--tv-Input-border-color)
- Both container and inner input use the same height variable
- While both elements use
box-sizing: border-box
, the inner input hasborder: 0
This means:
- Container height = specified height (includes 1px border top + 1px border bottom)
- Inner input height = specified height (no borders)
Recommendations:
- Either subtract 2px from the container height to account for borders
- Or add corresponding borders to the inner input
- Or adjust the
--tv-Input-small-height
variable to account for the border differenceThe font-size changes alone won't fix this structural box model issue.
🔗 Analysis chain
Line range hint
258-262
: Verify if font-size changes fully address the height issue.While adjusting the font size can affect height calculations, it's unclear if this alone resolves the 1px height difference between the input container and input. Consider:
- Explicitly verifying the computed heights after this change
- Checking if additional adjustments to height/line-height are needed
Let me try to find more specific information about the input sizing and height calculations.
Font-size changes alone may not fully address the height misalignment
Based on the codebase analysis, the height alignment between input container and input is controlled by multiple factors:
- The input container and inner input both use
var(--tv-Input-height)
for height and line-height- The
.input-size()
mixin is applied withvar(--tv-Input-small-height)
for small inputs- While font-size can affect text rendering height, the explicit height variables should be the primary control for component dimensions
The current changes only modify font-size without adjusting any height-related properties. Since both container and input already share the same height variable, the 1px difference might be caused by:
- Border calculations
- Box-sizing differences
- Padding/margin inconsistencies
Let's verify the box model properties:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for height-related properties that might affect the container/input alignment rg -l "height|line-height" --type lessLength of output: 141
Script:
#!/bin/bash # Search for height-related properties in the input less file rg "height|line-height" "packages/theme/src/input/index.less" # Also search for the input-size mixin definition ast-grep --pattern 'mixin input-size($_) { $$$ }'Length of output: 670
Script:
#!/bin/bash # Search for any CSS variable definitions related to input heights rg "tv-Input.*height|tv-Input.*line-height" "packages/theme/src/input"Length of output: 1258
Script:
#!/bin/bash # Look for the small input class definition rg "input.*small|small.*input" "packages/theme/src/input/index.less" -A 5 -B 5Length of output: 367
Script:
#!/bin/bash # Search for border, padding, margin, and box-sizing properties rg "border|padding|margin|box-sizing" "packages/theme/src/input/index.less" # Also check input-related variables rg "tv-Input.*(border|padding|margin)" "packages/theme/src/input/vars.less"Length of output: 2268
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/theme/src/input/index.less
(2 hunks)
🔇 Additional comments (1)
packages/theme/src/input/index.less (1)
Line range hint 258-273
: Additional investigation needed for height alignment.
The current changes focus on font-size adjustments, which might indirectly affect height calculations. However, to fully address the 1px height difference:
- Consider investigating the computed box model (margins, paddings, borders) of both container and input
- You might need to adjust height-related properties directly
- Add regression tests to ensure consistent heights across different size variants
#!/bin/bash
# Search for height-related mixins that might need adjustment
ast-grep --pattern 'mixin input-size($_) {
$$$
}'
…2490
PR
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: N/A
What is the new behavior?
Does this PR introduce a breaking change?
Other information
close #2490
Summary by CodeRabbit
small
andmini
input elements to ensure visual consistency.