Skip to content
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

[TextareaAutosize] Sync height when the width of the textarea changes #27840

Merged
merged 3 commits into from
Aug 26, 2021

Conversation

hbjORbj
Copy link
Member

@hbjORbj hbjORbj commented Aug 19, 2021

Closes #23641

Problem: as mentioned in the issue, change in the width of a textarea currently does not update the height. Hence, unwanted whitespace ends up existing in multi-line textarea.

Solution: use ResizeObserver API to observe the textarea and update the height in response to the textarea's resizing.

Consider the following textarea:
Screenshot 2021-08-19 at 08 45 54

Without the implementation of this pull request (when there is change in the width of the textarea):
Screenshot 2021-08-19 at 08 46 01

With the implementation of this pull request (when there is change in the width of the textarea):
Screenshot 2021-08-19 at 08 51 06

@hbjORbj hbjORbj added bug 🐛 Something doesn't work component: TextareaAutosize The React component. labels Aug 19, 2021
@hbjORbj hbjORbj requested review from mnajdova and eps1lon August 19, 2021 07:59
@mui-pr-bot
Copy link

mui-pr-bot commented Aug 19, 2021

Details of bundle changes (experimental)

Generated by 🚫 dangerJS against b4a0777

Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. I think that we can keep the window resize handler, for when ResizeObserver is not defined
  2. I'm no longer sure we need a useResizeObserver abstraction. The usage of the native API seems simple enough. If we need to debounce, the story will be different.

Comment on lines 134 to 138
try {
resizeObserverRef.current = new ResizeObserver(handleResize);
} catch (err) {
resizeObserverRef.current = MockResizeObserver(); // Prevent crash for old browsers
}
Copy link
Member

@oliviertassinari oliviertassinari Aug 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to try catch nor to create a MockResizeObserve. We can test if ResizeObserver is defined and do nothing if it's not defined.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This also prevents test failure, so I think we still need to include it?

Copy link
Member

@eps1lon eps1lon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can't be merged at the moment. We need to wait for the next major version to communicate new browser support.

@oliviertassinari
Copy link
Member

oliviertassinari commented Aug 23, 2021

This can't be merged at the moment. We need to wait for the next major version to communicate new browser support.

@eps1lon

  1. Could you expand on why we would need to wait for a new major?
  2. What if we were making this a progressive enhancement? Meaning, that if the developer targets a subset of the browsers supported by ResizeObserver it works. If the developer supports a wider range of browsers, then we have 3 options, could either say a. broken as before. b. use a polyfill c. expose an action prop with an imperative API as we do for the <Tabs>. In each case, we could cover this as a "Limitation" in the docs, e.g. for IE 11.

Copy link
Member

@eps1lon eps1lon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Way simpler, looks great!

@eps1lon
Copy link
Member

eps1lon commented Aug 25, 2021

Could you expand on why we would need to wait for a new major?

The legacy bundle would've no longer worked in IE11.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: TextareaAutosize The React component.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[TextareaAutosize] Sync height when the width of the textarea changes
4 participants