ref: growing input is no more #86553
Merged
Conversation
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
github-actions
bot
added
the
Scope: Frontend
JonasBa
commented
Mar 6, 2025
Comment on lines
62
to
66
| const computedStyles = window.getComputedStyle(input); | ||
|
|
||
| const sizingDiv = createSizingDiv(computedStyles); | ||
| sizingDiv.innerText = input.value || input.placeholder; | ||
| document.body.appendChild(sizingDiv); |
There was a problem hiding this comment.
Seems pretty wasteful to append a new child per input, but I'm holding off on making wider changes here.
There was a problem hiding this comment.
this is creating a new child per input per resize though, right? could be a performance nightmare
There was a problem hiding this comment.
Yup, exactly! It's been doing that before too, so that'd be a great case to go for the pure CSS solution you shared!
JonasBa
commented
Mar 6, 2025
| document.body.appendChild(sizingDiv); | ||
|
|
||
| const newTotalInputSize = | ||
| sizingDiv.offsetWidth + |
There was a problem hiding this comment.
I wonder if we can just use calc(${n}ch + padding instead of querying the width. If we can, then we might be able to remove other parts of the sizing functionality as well
There was a problem hiding this comment.
Update: we cant because ch unit is apparently based on the the 0 character. When I tested this, I was seeing a drift between the input and width which grew with the size of the input
❌ 1 Tests Failed:
View the top 1 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
JonasBa
force-pushed
the
jb/components/growing-input
branch
from
c715e8c to
b2dc249
Compare
JonasBa
commented
Mar 7, 2025
| @@ -89,6 +89,7 @@ export default storyBook('ComboBox', story => { | |||
| }); | |||
|
|
|||
| story('With growing input', () => { | |||
| const [value, setValue] = useState('opt_one'); | |||
There was a problem hiding this comment.
No value here meant that selecting an option in the story result in a weird flash and the combobox to not contain the clicked value
Comment on lines
14
to
17
| options: { | ||
| disabled?: boolean; | ||
| value?: React.InputHTMLAttributes<HTMLInputElement>['value'] | undefined; | ||
| } = {} |
There was a problem hiding this comment.
nit: move the types to a interface Options or something
evanpurkhiser
approved these changes
Mar 19, 2025
|
I need to figure out what broke here before I can merge this |
|
This issue has gone three weeks without activity. In another week, I will close it. But! If you comment or otherwise update it, I will reset the clock, and if you remove the label "A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀 |
|
I finally got back to fixing this, but I just missed attaching autosize to those inputs that broke in the screenshot above. Tested and things look good. @malwilley and @scttcper, I tagged you two because the change is primarily to the combobox searchbar, and that is a critical component. I tested things locally, and everything seems to work as it used to |
malwilley
approved these changes
Jun 17, 2025
|
Thank you for the reviews 🙇🏼 |
andrewshie-sentry
pushed a commit
that referenced
this pull request
Jun 19, 2025
All bow to useAutosizeInput and `<Input autosize/>` I have opted to use <Input autosize as opposed to requiring everyone to call the hook, as this allows us to ensure that the hook is correctly called (value is required depending on if the input is controlled or not), meaning that we can reduce the possible bug surface. https://github.com/user-attachments/assets/a321da42-e648-42c7-ad94-e885ba99bb12 The PR is missing tests, I had started to write some and realized mid way that I will have to mock pretty much all of the window styles methods, defeating the purpose of the tests (I will give that another go) The GrowingInput component was also hiding a bug that caused react to not throw warnings in development when only the value prop was provided as it was providing its own onchange handler and then calling `props.onChange?.()` which was always falsy **Below** is what I originally typed into the draft PR and explains the reasoning for ^ decision more in depth. The question now is, do we expose this via hook, or do we build it into the Input via autosize prop like `<Input autosize/>`? I think I am leaning towards autosize prop as I believe it is more intuitive than consuming a hook (which could still be exposed). If we require users to call the hook, then the API would look something like ```tsx function Component() { const ref = useAutosizeInput({value}) return <Input ref={ref}/> } ``` The awkward part is that exposing it like this could that the DOM structure could depend on the autosize prop (provided would use the CSS only approach that @natemoo-re shared, which requires a wrapper component). An alternative could be to add autosize to InputGroup and require the prop to be set there, but that feels weird, as the autosize input should be set on the input itself, and making InputGroup a requirement for autosize to work feels sub-optimal. ```tsx function Component(){ return ( <InputGroup autosize> // <- this feels weird, but also means that standalone inputs dont support autosize? <InputGroup.Input/> </InputGroup> } ``` Finally, and the version that I am most leaning towards is to expose this as an autosize bool prop on Input, and use the current JS handler to measure resizing. This way, we can also ensure that the hook is correctly called with the value props if the component is controlled, and hide that last bit of complexity from the user (provided we dont use the pure CSS solution for now) ```tsx function Component() { return <Input autosize/> } ``` @natemoo-re @TkDodo @evanpurkhiser or anyone else reading this, feedback and opinions would be much appreciated
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.
All bow to useAutosizeInput and
<Input autosize/>I have opted to use <Input autosize as opposed to requiring everyone to call the hook, as this allows us to ensure that the hook is correctly called (value is required depending on if the input is controlled or not), meaning that we can reduce the possible bug surface.
CleanShot.2025-03-07.at.09.48.15.mp4
The PR is missing tests, I had started to write some and realized mid way that I will have to mock pretty much all of the window styles methods, defeating the purpose of the tests (I will give that another go)
The GrowingInput component was also hiding a bug that caused react to not throw warnings in development when only the value prop was provided as it was providing its own onchange handler and then calling
props.onChange?.()which was always falsyBelow is what I originally typed into the draft PR and explains the reasoning for ^ decision more in depth.
The question now is, do we expose this via hook, or do we build it into the Input via autosize prop like
<Input autosize/>? I think I am leaning towards autosize prop as I believe it is more intuitive than consuming a hook (which could still be exposed).If we require users to call the hook, then the API would look something like
The awkward part is that exposing it like this could that the DOM structure could depend on the autosize prop (provided would use the CSS only approach that @natemoo-re shared, which requires a wrapper component). An alternative could be to add autosize to InputGroup and require the prop to be set there, but that feels weird, as the autosize input should be set on the input itself, and making InputGroup a requirement for autosize to work feels sub-optimal.
Finally, and the version that I am most leaning towards is to expose this as an autosize bool prop on Input, and use the current JS handler to measure resizing. This way, we can also ensure that the hook is correctly called with the value props if the component is controlled, and hide that last bit of complexity from the user (provided we dont use the pure CSS solution for now)
@natemoo-re @TkDodo @evanpurkhiser or anyone else reading this, feedback and opinions would be much appreciated