-
-
Notifications
You must be signed in to change notification settings - Fork 32.3k
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
[TextField] Fix required outlined label space with no asterisk #20715
Conversation
Details of bundle changes.Comparing: 79a29f5...e442877 Details of page changes
|
@@ -107,10 +107,11 @@ const TextField = React.forwardRef(function TextField(props, ref) { | |||
InputMore.notched = InputLabelProps.shrink; | |||
} | |||
if (label) { | |||
const displayRequired = InputLabelProps?.required ?? required; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line transforms into:
var _InputLabelProps$requ, _InputLabelProps;
var displayRequired = (_InputLabelProps$requ = (_InputLabelProps = InputLabelProps) === null ||
_InputLabelProps === void 0 ? void 0 : _InputLabelProps.required) !== null
&& _InputLabelProps$requ !== void 0 ? _InputLabelProps$requ : required;
What do you think of?
const displayRequired = InputLabelProps?.required ?? required; | |
const displayRequired = InputLabelProps && InputLabelProps.required != null ? required : undefined; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's stick with the correct semantics and improve transpiling with modern bundles.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder about the tradeoff:
- cost: what's the broader impact if we fully scale-out this syntax? +X% bundle size impact, will X be negligible? I wonder about why the transpiled code tests for strict equality with undefined and null vs
!= null
. Maybe Babel can/will optimize it. I hope it won't, at scale, have the same overhead has theclass
syntax. - value: what cases does the new syntax allow us to better cover?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's the broader impact if we fully scale-out this syntax?
Valid point. But we need to be careful that we don't fall into worrying about things we don't need (see "yagni").
I'm looking through all our plugins and see if we can safely enable loose mode in preset-env. For only these two syntaxes babel/babel#6978 would help.
Closes #20705
Fix verified in https://codesandbox.io/s/asterisk-material-ui-4911-uheri