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.

Sign up for GitHub

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

Jump to bottom

[InputAdornment][material-next] Add InputAdornment component #39722

Closed
wants to merge 18 commits into from
Closed

[InputAdornment][material-next] Add InputAdornment component #39722

wants to merge 18 commits into from

Conversation

mj12albert
Copy link
Member

@mj12albert mj12albert commented Nov 2, 2023 •
edited
Loading

Closes: #39628
Could also fix: #13898

Changes

@mj12albert mj12albert added component: text field This is the name of the generic UI component, not the React module! design: material you labels Nov 2, 2023
@mui-bot
Copy link

mui-bot commented Nov 2, 2023 •
edited
Loading

Netlify deploy preview

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

@mui/material-next: parsed: +0.54% , gzip: +0.27%

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against a1c2696

@mj12albert
Copy link
Member Author

Waiting for #39483 to be merged before working on the layout changes involving the label and start adornment

@mj12albert mj12albert force-pushed the material-next/input-adornment branch 4 times, most recently from 451c929 to 26718f7 Compare November 10, 2023 10:05
@mj12albert mj12albert force-pushed the material-next/input-adornment branch 3 times, most recently from beb2161 to 2f7a97c Compare November 25, 2023 05:22
@mj12albert mj12albert force-pushed the material-next/input-adornment branch 2 times, most recently from 03bc8bd to d4032a9 Compare December 20, 2023 09:56
@mj12albert mj12albert force-pushed the material-next/input-adornment branch 4 times, most recently from 6413e92 to 6662291 Compare December 28, 2023 07:21
@mj12albert mj12albert force-pushed the material-next/input-adornment branch from 7b0c65b to 3f66cba Compare January 2, 2024 06:55
@mj12albert mj12albert force-pushed the material-next/input-adornment branch from 3f66cba to 11e584c Compare January 2, 2024 09:02
@mj12albert
Copy link
Member Author

Would like to ask @zanivan & @DiegoAndai for an initial review of the design changes in this PR (details here)

I've added some knobs on the experiments page to toggle FormHelperText (the checkbox) and the color prop (the select) of the components on the whole page

The code is also close to review-able, it could use an extra round of polish after the design is reviewed and there may be some CSS that could be further tokenized into css variables

Demo: https://deploy-preview-39722--material-ui.netlify.app/experiments/md3/inputs/

Screenshot 2024-01-02 at 9 37 47 PM

@mj12albert mj12albert marked this pull request as ready for review January 2, 2024 13:46
mj12albert
mj12albert commented Jan 2, 2024
View reviewed changes
packages/mui-material-next/src/InputLabel/InputLabel.tsx
...(ownerState.formControl?.adornedStart &&
!ownerState.formControl?.adornedEnd && {
transform: 'translate(52px, 7px) scale(0.75)',
maxWidth: 'calc(133% - 32px - 48px - 8px)',
Copy link
Member Author

Choose a reason for hiding this comment

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

When it's in the shrink state, the correct maxWidth adjustment seems to be "adjust by 48px per adornment, then adjust by another 8px", though I'm not 100% sure if this is a real "pattern" or if my mind is just trying to fill in the gaps 😅

Copy link
Member

Choose a reason for hiding this comment

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

😅 Where do you think the additional 8 might come from?

DiegoAndai
DiegoAndai reviewed Jan 2, 2024
View reviewed changes
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.

Overall looks good 😊
Let me know if you need brainstorming on the CSS variables or structure.
I'll do the final review after any design changes required.

packages/mui-material-next/src/InputAdornment/InputAdornment.test.tsx

expect(adornment).to.have.class(classes.root);
expect(adornment).to.have.class(classes.positionEnd);
});

describe('prop: variant', () => {
it("should inherit the TextField's variant", () => {
// TODO v6: requires material-next/TextField
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// TODO v6: requires material-next/TextField
// TODO v7: requires material-next/TextField

packages/mui-material-next/src/InputAdornment/InputAdornment.test.tsx
@@ -88,7 +104,9 @@ describe('<InputAdornment />', () => {
expect(adornment).to.have.class(classes.filled);
});

it('should override the inherited variant', () => {
// TODO v6: requires material-next/TextField
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// TODO v6: requires material-next/TextField
// TODO v7: requires material-next/TextField

packages/mui-material-next/src/InputLabel/InputLabel.tsx
Comment on lines +111 to +113
...(ownerState.formControl?.adornedStart && {
transform: 'translate(52px, 13px) scale(1)',
}),
Copy link
Member

Choose a reason for hiding this comment

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

Why only for the adornedStart?

packages/mui-material-next/src/InputLabel/InputLabel.tsx
@@ -88,17 +88,61 @@ const InputLabelRoot = styled(FormLabel, {
pointerEvents: 'none',
transform: 'translate(16px, 16px) scale(1)',
maxWidth: 'calc(100% - 24px)',
Copy link
Member

Choose a reason for hiding this comment

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

What are the base - 24px for?

packages/mui-material-next/src/InputLabel/InputLabel.tsx
...(ownerState.formControl?.adornedStart &&
!ownerState.formControl?.adornedEnd && {
transform: 'translate(52px, 7px) scale(0.75)',
maxWidth: 'calc(133% - 32px - 48px - 8px)',
Copy link
Member

Choose a reason for hiding this comment

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

😅 Where do you think the additional 8 might come from?

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 3, 2024
zanivan
zanivan reviewed Jan 11, 2024
View reviewed changes
packages/mui-material-next/src/InputAdornment/InputAdornment.tsx
color: 'var(--md-comp-input-adornment-color)',
...(ownerState.position === 'start' && {
// Styles applied to the root element if `position="start"`.
marginRight: 16,
Copy link
Contributor

Choose a reason for hiding this comment

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

Checking the design guidelines from the design resources of MD3, this distance is actually 12px instead of 16px

image

@mj12albert mj12albert closed this Mar 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zanivan zanivan zanivan left review comments

@DiegoAndai DiegoAndai DiegoAndai left review comments

Assignees
No one assigned
Labels
component: text field This is the name of the generic UI component, not the React module! design: material you PR: out-of-date The pull request has merge conflicts and can't be merged v7.x
Projects
None yet
Milestone
No milestone
4 participants
@mj12albert @mui-bot @DiegoAndai @zanivan