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

[base-ui][material-ui][TextareaAutosize] Fix inline style not getting applied #41369

Merged

Conversation

ZeeshanTamboli
Copy link
Member

@ZeeshanTamboli ZeeshanTamboli commented Mar 5, 2024

Fix part of mui/base-ui#168 where style was not getting applied. It's a regression from #40789.

Before: https://codesandbox.io/p/sandbox/magical-fermi-nctqps
After: https://codesandbox.io/p/sandbox/compassionate-napier-7tnq6l

@ZeeshanTamboli ZeeshanTamboli added bug 🐛 Something doesn't work package: material-ui Specific to @mui/material component: TextareaAutosize The React component. package: base-ui Specific to @mui/base regression A bug, but worse labels Mar 5, 2024
@ZeeshanTamboli ZeeshanTamboli marked this pull request as ready for review March 5, 2024 12:40
@mui-bot
Copy link

mui-bot commented Mar 5, 2024

Netlify deploy preview

https://deploy-preview-41369--material-ui.netlify.app/

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against 5c85931

Copy link
Member

@DiegoAndai DiegoAndai left a comment

Choose a reason for hiding this comment

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

Hey @ZeeshanTamboli, I have a single suggestion.

@ZeeshanTamboli ZeeshanTamboli merged commit 27b8c76 into mui:master Mar 8, 2024
24 checks passed
@ZeeshanTamboli ZeeshanTamboli deleted the fix-part-of-41315-textareaautosize branch March 8, 2024 16:36
@oliviertassinari
Copy link
Member

oliviertassinari commented Mar 10, 2024

We recently committed to only move forward from Base UI repository https://github.com/mui/base-ui so this PR isn't solving the problem. I transferred the issue to Base UI mui/base-ui#168. It's great that we didn't close it, the problem is not truly fixed yet 👍.

Next, we still need to:

Comment on lines +467 to +468
const { container } = render(<TextareaAutosize style={{ backgroundColor: 'yellow' }} />);
const input = container.querySelector<HTMLTextAreaElement>('textarea')!;
Copy link
Member

@oliviertassinari oliviertassinari Mar 10, 2024

Choose a reason for hiding this comment

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

Prefer using the screen. We are moving tests as much as possible to rely on global queries. This is purely to keep the test environment simple. Most of the time, we don't need the notion of a container. We render one element at once on the screen.

Suggested change
const { container } = render(<TextareaAutosize style={{ backgroundColor: 'yellow' }} />);
const input = container.querySelector<HTMLTextAreaElement>('textarea')!;
render(<TextareaAutosize style={{ backgroundColor: 'yellow' }} />);
const input = document.querySelector<HTMLTextAreaElement>('textarea')!;

@ZeeshanTamboli
Copy link
Member Author

We recently committed to only move forward from Base UI repository https://github.com/mui/base-ui so this PR isn't solving the problem. I transferred the issue to Base UI mui/base-ui#168. It's great that we didn't close it, the problem is not fixed 👍.

Next, we still need to:

@oliviertassinari Noted. However, addressing all of this will require some time, correct? Given that this was a regression, it was crucial to implement an urgent fix in this repository for the upcoming release. Nevertheless, I agree that the fix should also be applied in the Base UI repository - mui/base-ui#177.

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. package: base-ui Specific to @mui/base package: material-ui Specific to @mui/material regression A bug, but worse
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants