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

[FormControl] Add useFormControlState #16467

Merged
merged 3 commits into from
Jul 5, 2019

Conversation

eps1lon
Copy link
Member

@eps1lon eps1lon commented Jul 3, 2019

Going to come up with an example later. The point was to rewrite the test so that we don't rely on the internal FormControlContext.Provider and the onEmpty/onFilled callcounts. It's very likely that they will change soon anyway and I don't want to battle with internals. The important bit was that filled is propagated not how many times these handlers were called (since nobody taps into them anyway and we accepted buggy behavior).

Closes #11865

Follow-up:

  • replace withFormControl with useFormControl (deferred since no immediate benefit).

@eps1lon eps1lon added new feature New feature or request component: FormControl labels Jul 3, 2019
@mui-pr-bot
Copy link

mui-pr-bot commented Jul 3, 2019

Details of bundle changes.

Comparing: 73e9950...4c6b8dd

bundle parsed diff gzip diff prev parsed current parsed prev gzip current gzip
@material-ui/core 0.00% +0.02% 🔺 319,998 320,008 88,297 88,314
@material-ui/core/Paper 0.00% 0.00% 68,292 68,292 20,371 20,371
@material-ui/core/Paper.esm 0.00% 0.00% 61,574 61,574 19,162 19,162
@material-ui/core/Popper 0.00% 0.00% 28,945 28,945 10,401 10,401
@material-ui/core/Textarea 0.00% 0.00% 5,507 5,507 2,362 2,362
@material-ui/core/TrapFocus 0.00% 0.00% 3,753 3,753 1,579 1,579
@material-ui/core/styles/createMuiTheme 0.00% 0.00% 16,010 16,010 5,790 5,790
@material-ui/core/useMediaQuery 0.00% 0.00% 2,597 2,597 1,103 1,103
@material-ui/lab 0.00% 0.00% 140,286 140,286 43,459 43,459
@material-ui/styles 0.00% 0.00% 51,699 51,699 15,346 15,346
@material-ui/system 0.00% 0.00% 15,420 15,420 4,390 4,390
Button 0.00% 0.00% 84,443 84,443 25,723 25,723
Modal 0.00% 0.00% 14,534 14,534 5,087 5,087
Portal 0.00% 0.00% 3,473 3,473 1,573 1,573
Slider 0.00% 0.00% 75,108 75,108 23,306 23,306
colorManipulator 0.00% 0.00% 3,904 3,904 1,544 1,544
docs.landing 0.00% 0.00% 54,338 54,338 13,762 13,762
docs.main +0.01% 🔺 +0.01% 🔺 646,957 647,028 203,886 203,914
packages/material-ui/build/umd/material-ui.production.min.js 0.00% 0.00% 292,982 292,982 84,082 84,082

Generated by 🚫 dangerJS against 4c6b8dd

@@ -1 +1,2 @@
export { default } from './FormControl';
export { useFormControl } from './FormControlContext';
Copy link
Member

Choose a reason for hiding this comment

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

What do you think of merging this hook with the formControlState helper? #11865 (comment)

Copy link
Member

@oliviertassinari oliviertassinari Jul 4, 2019

Choose a reason for hiding this comment

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

I mean, what's the best API to expose publicly?

Copy link
Member Author

Choose a reason for hiding this comment

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

Just expose the state? Why do you think formControlState should be included?

Copy link
Member

@oliviertassinari oliviertassinari Jul 4, 2019

Choose a reason for hiding this comment

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

I personally have a slight preference for not exposing formControlState. This is a module we use for merging the context with the props values. People asking for accessing the form state might not need it (if they do, they can handle it on their side). However, I'm asking because people tracking #11865 might expect formControlState to be public based on my previous comment. Should we collect their feedback?

Ok, maybe we can go for it, collect feedback once released, the future cost of change will be minimal.

@eps1lon eps1lon merged commit 7050cf5 into mui:master Jul 5, 2019
@eps1lon eps1lon deleted the feat/useFormControlState branch July 5, 2019 09:50
@oliviertassinari oliviertassinari added the component: text field This is the name of the generic UI component, not the React module! label Apr 24, 2020
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! new feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FormControl] Expose the form control state
3 participants