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

[Input][material-next] Add InputBase component #38392

Merged
merged 23 commits into from
Aug 22, 2023

Conversation

mj12albert
Copy link
Member

@mj12albert mj12albert commented Aug 9, 2023

Part of #38372

This PR ports InputBase from Material UI to material-next, converts it to TypeScript, and replaces as much functionality as possible with Base UI's useInput.

Additionally:

  • Styles are changed to consume MD3 tokens from CssVarsProvider (though all the styles are style resets)
  • inputComponent, inputProps, components, and componentsProps are dropped, related features and tests are refactored to use slots/slotProps
  • FormControl integration and related tests are skipped in this PR, I think this is as far as we can get with InputBase without it

@mj12albert mj12albert added v6.x component: input This is the name of the generic UI component, not the React module! labels Aug 9, 2023
@mui-bot
Copy link

mui-bot commented Aug 9, 2023

Netlify deploy preview

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

@mui/material-next: parsed: +2.55% , gzip: +1.53%

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against b0c11a2

@mj12albert mj12albert force-pushed the material-next/input-base branch 8 times, most recently from f2b6c70 to daca6d3 Compare August 10, 2023 15:51
@mj12albert mj12albert force-pushed the material-next/input-base branch 2 times, most recently from 4d635cd to b6818bc Compare August 11, 2023 09:11
@@ -20,4 +22,29 @@ describe('useInput', () => {
expect(inputRef.current).to.deep.equal(getByRole('textbox'));
});
});

describe('prop: disabled', () => {
it('should reset the focused state if getting disabled', () => {
Copy link
Member Author

Choose a reason for hiding this comment

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

The code related to this used to be in InputBase here but it's now implemented in useInput, so I figure it's probably better to test it here? @michaldudak

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, let's test the hooks extensively so that other products can just assume they work correctly and focus on testing only the additional functionality they provide.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds good – I'll add a task to port additional relevant tests to Base in the issue

@mj12albert mj12albert marked this pull request as ready for review August 11, 2023 09:26
@mj12albert mj12albert force-pushed the material-next/input-base branch 3 times, most recently from f4e1e12 to 8468f94 Compare August 14, 2023 06:50
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.

Looks good to me! Only have two questions regarding the test file

const [flag, setFlag] = React.useState(true);

return (
<FormControl>
Copy link
Member

Choose a reason for hiding this comment

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

Was this removed due to the use of FormControl? Or the use of Select? Or is there another reason?

Copy link
Member Author

@mj12albert mj12albert Aug 16, 2023

Choose a reason for hiding this comment

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

It's because of Select, but now I'm wondering if this test should be moved to FormControl.test instead

"toggling between inputs (inside a FormControl)" could still be a valid case even the select prop on TextField is dropped in favor of a SelectField component (#21782)

What do you think? @DiegoAndai

Copy link
Member

Choose a reason for hiding this comment

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

I thought the same; we should move it to FormControl.test when we migrate it. It's okay to remove it here, but let's write it down somewhere so we remember to bring it back later.

Copy link
Member Author

@mj12albert mj12albert Aug 16, 2023

Choose a reason for hiding this comment

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

👍 I've added it as a task to #38411

packages/mui-material-next/src/InputBase/InputBase.test.js Outdated Show resolved Hide resolved
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.

This looks ready for me, let's ask @michaldudak @mnajdova if they wish to review it before merging.

Nice work 🎉

@siriwatknp
Copy link
Member

siriwatknp commented Aug 22, 2023

Is there a doc page for this component to see what it looks like? If not, I will merge it.

@mj12albert
Copy link
Member Author

Is there a doc page for this component to see what it looks like? If not, I will merge it.

@siriwatknp Thanks for checking this ~ I think for material-next there are no docs yet besides migration.md

@siriwatknp siriwatknp enabled auto-merge (squash) August 22, 2023 10:00
@siriwatknp siriwatknp merged commit ab8f160 into mui:master Aug 22, 2023
5 of 7 checks passed
@mj12albert mj12albert deleted the material-next/input-base branch August 28, 2023 07:24
@DiegoAndai DiegoAndai added v7.x and removed v6.x labels Dec 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: input This is the name of the generic UI component, not the React module! v7.x
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants