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] Fix resizing instability #189

Merged

Conversation

ZeeshanTamboli
Copy link
Member

@ZeeshanTamboli ZeeshanTamboli commented Mar 16, 2024

Fixes #168
Fixes #160

Thanks to @hyorimitsu for pinpointing the cause of the issue, as mentioned in #168 (comment).

The solution involves storing the height in a ref and updating it only when it differs from the previous value. Previously, we set the height everytime even if it remained unchanged, causing the glitch.

Before: https://codesandbox.io/p/sandbox/old-wave-py2jkq
After: https://codesandbox.io/p/sandbox/admiring-dan-9kqmtk


If this looks good, should I open a PR in the Material UI repository as well? This fix addresses a regression, and I'm uncertain about when it will be released from this repository.

@ZeeshanTamboli ZeeshanTamboli added component: textarea This is the name of the generic UI component, not the React module! bug 🐛 Something doesn't work labels Mar 16, 2024
@ZeeshanTamboli ZeeshanTamboli changed the title [TextareaAutosize] Fix resizing unstability [TextareaAutosize] Fix resizing instability Mar 16, 2024
@ZeeshanTamboli ZeeshanTamboli marked this pull request as ready for review March 17, 2024 09:15
@ZeeshanTamboli ZeeshanTamboli added the regression A bug, but worse label Mar 17, 2024
@ZeeshanTamboli ZeeshanTamboli requested a review from a team March 17, 2024 09:19
@oliviertassinari oliviertassinari added the PR: needs test The pull request can't be merged label Mar 17, 2024
@ZeeshanTamboli
Copy link
Member Author

I'm having trouble coming up with a test for this. @oliviertassinari, since you added the "PR: needs test" label, could you assist me with it?

@michaldudak
Copy link
Member

If this looks good, should I open a PR in the Material UI repository as well? This fix addresses a regression, and I'm uncertain about when it will be released from this repository.

We are not going to release any new versions from this repo anytime soon (not unless we have a consistent API across existing components).

We decided not to encourage contributions to the Base UI in the Core repo either (so we don't end up managing two diverging versions), but since you already took the effort to fix the bug, we can release it from the Core repo.

I understand that it's not a perfect situation and we're doing what we can to have just one copy of the source code as soon as possible.

@ZeeshanTamboli
Copy link
Member Author

ZeeshanTamboli commented Mar 21, 2024

Since there is no one copy of the source code yet, I think we should fix it here in this repository too, or it will become out of sync. Once it's good here, I'll make a PR in the Core repository too. I think we should point it to the master branch instead of next in the Core repository and release it under v5. Later, we can merge the master branch with this fix into next. What are your thoughts on this? Who will release from the master branch and when?

@michaldudak
Copy link
Member

@DiegoAndai, @mnajdova - what would be the best way to release this from the Core repo?

@pirtlj
Copy link

pirtlj commented Mar 21, 2024

looking forward to seeing this fixed

@colmtuite
Copy link
Contributor

colmtuite commented Mar 21, 2024

Auto-sizing can be handled with field-sizing. It's not currently well-supported, though it should be later this year. So we're planning to deprecate this component for our v1 release later this year.

Here's a demo.

@DiegoAndai
Copy link
Member

what would be the best way to release this from the Core repo?

As discussed today in the core team meeting:

We can release whenever we have something to release. The process is to merge the change to next and then cherry-pick to master. Releases from master will only happen if there are changes.

@ZeeshanTamboli
Copy link
Member Author

Created PR in Core repo: mui/material-ui#41638.

@ZeeshanTamboli
Copy link
Member Author

ZeeshanTamboli commented Apr 26, 2024

@mui/base-ui I believe we should consider merging this. See the rationale in #168 (comment). The corresponding PR in the core repository (mui/material-ui#41638) has been merged, for existing @mui/base users and @mui/material users and those utilizing Material UI TextField with the multiline prop.

Regarding the test, I've implemented an E2E test in the Core repository. However, we currently lack the setup for E2E tests here. Should I proceed with setting up an E2E test suite, or should we consider merging this PR without a test?

@michaldudak
Copy link
Member

All right, we can merge it here as the future of this component is not clear yet. As for E2E tests, could you set them up in a separate PR?

@michaldudak michaldudak mentioned this pull request May 2, 2024
1 task
@ZeeshanTamboli
Copy link
Member Author

As for E2E tests, could you set them up in a separate PR?

I just saw it's being done in #395.

@michaldudak
Copy link
Member

All right, let's add the test once #395 is merged and we'll be able to close it.

@ZeeshanTamboli ZeeshanTamboli removed the PR: needs test The pull request can't be merged label May 8, 2024
@ZeeshanTamboli
Copy link
Member Author

@michaldudak Added test. Ready for merge.

@michaldudak michaldudak merged commit cab02f0 into mui:master May 8, 2024
18 checks passed
@ZeeshanTamboli ZeeshanTamboli deleted the issue-168-fix-resizing-unstable branch May 8, 2024 09:39
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: textarea This is the name of the generic UI component, not the React module! regression A bug, but worse
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[TextareaAutosize] Resizing unstable [TextareaAutosize] Glitchy resizing
6 participants