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

[FormHelperText][material-next] Add FormHelperText component #39503

Merged
merged 16 commits into from
Nov 23, 2023

Conversation

mj12albert
Copy link
Member

@mj12albert mj12albert commented Oct 18, 2023

Continues #38374
Closes #39513

This PR adds FormHelperText to material-next, I've updated the /experiments page again but also copied it to a sandbox 👇 for easier review ~

Demo: https://codesandbox.io/s/https-github-com-mui-material-ui-pull-39503-5f4k8n?file=/src/App.tsx

Preview:

material-next-form-helper-text.mov

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

mui-bot commented Oct 18, 2023

Netlify deploy preview

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

@mui/material-next: parsed: +0.39% , gzip: +0.15%

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against 1b6a6ae

@mj12albert mj12albert force-pushed the material-next/form-helper-text branch 3 times, most recently from e4f8b35 to 9477ae5 Compare October 26, 2023 10:24
@mj12albert mj12albert force-pushed the material-next/form-helper-text branch 2 times, most recently from e78c767 to 9c1add0 Compare November 1, 2023 10:55
@mj12albert

This comment was marked as outdated.

@mj12albert mj12albert force-pushed the material-next/form-helper-text branch 2 times, most recently from f36b5b9 to e6c8d1d Compare November 8, 2023 06:41
@mj12albert mj12albert added the v6.x label Nov 8, 2023
@mj12albert mj12albert force-pushed the material-next/form-helper-text branch 5 times, most recently from 78a7807 to 9949e22 Compare November 9, 2023 14:35
@mj12albert mj12albert force-pushed the material-next/form-helper-text branch from 9949e22 to b21b3f5 Compare November 10, 2023 09:23
@mj12albert mj12albert marked this pull request as ready for review November 10, 2023 09:41
@mj12albert mj12albert force-pushed the material-next/form-helper-text branch from 6e93751 to 33828b4 Compare November 10, 2023 09:52
@@ -67,7 +67,7 @@ export const FormLabelRoot = styled('label', {
},
[`&.${formLabelClasses.disabled}`]: {
color:
'color-mix(in srgb, var(--md-comp-form-label-disabled-color), transparent calc(var(--md-comp-form-label-disabled-opacity) * 100%))',
'color-mix(in srgb, var(--md-comp-form-label-disabled-color) calc(var(--md-comp-form-label-disabled-opacity) * 100%), transparent)',
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 % was on the wrong color from before but it was hard to see until now 😅

Comment on lines 82 to 83
marginLeft: 14,
marginRight: 14,
Copy link
Member

Choose a reason for hiding this comment

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

14 is an off number to end up with considering the spacing, is this based on the spec?

Copy link
Contributor

Choose a reason for hiding this comment

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

The Figma file uses 16px on both sides:

Screenshot 2023-11-21 at 10 42 52

However, if we change this, we must ensure that all labels are aligned:

Screenshot 2023-11-21 at 10 45 04

Copy link
Member Author

Choose a reason for hiding this comment

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

👍 I've realigned everything 5f9105d

(this is actually off in v5 as well, the MD2 spec also specifies 16px padding)

Copy link
Member

@mnajdova mnajdova 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, left one question, but it's likely me being paranoid :D

@zanivan
Copy link
Contributor

zanivan commented Nov 21, 2023

Looks good! ✨

While reviewing, noticed that the disabled aren't affected by opacity—shouldn't they be? 🤔

Screenshot 2023-11-21 at 10 37 51

@mj12albert
Copy link
Member Author

noticed that the disabled aren't affected by opacity

@zanivan Good catch! It was literally a typo 🤦 61052c7

Copy link
Contributor

@zanivan zanivan left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@mj12albert mj12albert merged commit c11715e into mui:master Nov 23, 2023
22 checks passed
@mj12albert mj12albert deleted the material-next/form-helper-text branch November 23, 2023 10:14
mnajdova pushed a commit to mnajdova/material-ui that referenced this pull request Nov 30, 2023
mnajdova pushed a commit to mnajdova/material-ui that referenced this pull request Dec 1, 2023
mnajdova pushed a commit to mnajdova/material-ui that referenced this pull request Dec 1, 2023
mnajdova pushed a commit to mnajdova/material-ui that referenced this pull request Dec 6, 2023
mnajdova pushed a commit to mnajdova/material-ui that referenced this pull request Dec 6, 2023
mnajdova pushed a commit to mnajdova/material-ui that referenced this pull request Dec 6, 2023
@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: text field This is the name of the generic UI component, not the React module! design: material you v7.x
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FormHelperText][material-next] Add FormHelperText component
5 participants