fix(core): size height calculation - #204
Merged
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe select component now applies row-height and border calculations only in the ChangesControl and select sizing
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@projects/core/src/select/select.css`:
- Line 26: Add regression assertions in the select tests covering computed
height for a default single-select, a sized select, and after clearing size.
Extend the existing :state(size)/--size coverage near the referenced select test
cases, verifying both the --height fallback and the row/gap/border calculation
paths.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: fcf7a93d-1545-4133-89d0-67cca415c785
📒 Files selected for processing (1)
projects/core/src/select/select.css
coryrylan
force-pushed
the
topic-input-style-fix
branch
from
July 31, 2026 20:54
b62dfa7 to
c174663
Compare
coryrylan
force-pushed
the
topic-input-style-fix
branch
from
August 4, 2026 14:48
c174663 to
3f104f0
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@projects/core/src/forms/control/control.test.ts`:
- Around line 272-305: Add a test alongside the existing fit-text sizing cases
that exercises a select element and asserts its rendered width changes when the
selected content or available options change. Reuse the existing control setup
and stabilization/animation-frame waits, and verify the select’s measured width
before and after the change to cover the field-sizing behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: cca8720c-f81b-4c22-a3e3-339eea9bead9
⛔ Files ignored due to path filters (2)
projects/core/.visual/toolbar.dark.pngis excluded by!**/*.pngprojects/core/.visual/toolbar.pngis excluded by!**/*.png
📒 Files selected for processing (4)
projects/core/src/forms/control/control.cssprojects/core/src/forms/control/control.test.tsprojects/core/src/forms/control/control.tsprojects/core/src/input/input.css
💤 Files with no reviewable changes (2)
- projects/core/src/input/input.css
- projects/core/src/forms/control/control.ts
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.
Suppressed comments (3)
projects/core/src/forms/control/control.css:64
- The new
:host([fit-text]) { ::slotted(...) { ... } }block relies on CSS nesting. If nesting isn’t consistently supported/processed in this repo’s CSS pipeline, the inner rule will be ignored andfit-textwon’t apply. Prefer explicit selectors (and optionally guardfield-sizingbehind@supports) to make the styling unambiguous.
:host([fit-text]) {
--max-width: fit-content;
--width: fit-content;
::slotted(input),
::slotted(select) {
field-sizing: content;
}
}
projects/core/src/forms/control/control.ts:183
- This PR is titled/described as a select
sizeheight calculation fix, but it also removes thefit-textruntime sizing logic fromControl(and derived components likenve-input/nve-select) and rewrites the related unit tests. That’s a behavior/API change beyond the stated scope, so it should either be called out in the PR description/release notes or split into a separate PR to keep the change focused.
#onRootSlotchange = () => {
this.#updateStyleStates();
if (this.input && this.#observers.length === 0) {
this.#setupInput();
}
projects/core/src/forms/control/control.test.ts:336
- The dedicated
fit-text selectunit tests were removed. Sincefit-textis still documented/implemented for select (via::slotted(select)styling), this reduces coverage for a supported code path and makes regressions harder to catch. Consider restoring equivalent assertions (or adding new ones that validatenve-select fit-textbehavior) so both input and select are exercised.
describe(`${Control.metadata.tag}: fit-content input`, () => {
let fixture: HTMLElement;
let element: Control;
let input: HTMLInputElement;
beforeEach(async () => {
fixture = await createFixture(html`
<nve-control fit-content>
<label>label</label>
<input type="text" />
</nve-control>
`);
element = fixture.querySelector(Control.metadata.tag);
input = fixture.querySelector('input');
await elementIsStable(element);
});
afterEach(() => {
removeFixture(fixture);
});
it('should update control width to input browser default content', async () => {
await elementIsStable(element);
await new Promise(r => requestAnimationFrame(r));
expect(Math.floor(input.getBoundingClientRect().width) > 100).toBe(true);
expect(Math.floor(input.getBoundingClientRect().width) < 250).toBe(true);
});
});
- only set explicit computed height when size is applied, prevents specificity and rounding errors on standard single selects Signed-off-by: Cory Rylan <crylan@nvidia.com>
coryrylan
force-pushed
the
topic-input-style-fix
branch
from
August 5, 2026 15:59
5bc62fd to
4d1c61f
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.
Suppressed comments (2)
projects/core/src/forms/control/control.css:64
fit-textrelies onfield-sizing: content, but several components (e.g.nve-inputandnve-select) set::slotted(input/select) { width: 100% !important; }, which prevents content-based sizing from taking effect. Consider overriding the slotted control width (and--control-width) within the:host([fit-text])rule sofield-sizingcan actually influence layout.
:host([fit-text]) {
--max-width: fit-content;
--width: fit-content;
::slotted(input),
projects/core/src/forms/control/control.test.ts:280
- The previous
fit-text selectcoverage was removed, butfit-textis still expected to apply to<select>(Control CSS now targets::slotted(select)). With no remaining unit coverage for the select case, regressions (especially around select sizing) are unlikely to be caught. Consider adding at least one deterministic test for the select path (e.g., asserting a measurable width change between two option labels, or asserting thefield-sizingcomputed style when supported).
it('should set input width to text content width', async () => {
await new Promise(resolve => requestAnimationFrame(resolve));
const fitTextWidth = input.getBoundingClientRect().width;
element.fitText = false;
await elementIsStable(element);
await new Promise(resolve => requestAnimationFrame(resolve));
expect(fitTextWidth).toBeLessThan(input.getBoundingClientRect().width);
Signed-off-by: Cory Rylan <crylan@nvidia.com>
coryrylan
force-pushed
the
topic-input-style-fix
branch
from
August 5, 2026 17:23
4d1c61f to
9ab9bca
Compare
johnyanarella
approved these changes
Aug 5, 2026
Collaborator
Author
|
🎉 This issue has been resolved in version 2.2.3 🎉 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
fit-textvariant.Bug Fixes