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

Implemented custom AutoField prototype. #800

Merged
merged 5 commits into from
Feb 3, 2021
Merged

Conversation

radekmie
Copy link
Contributor

@radekmie radekmie commented Sep 10, 2020

This change provides a new implementation of AutoField and an according AutoFieldContext to override field detection implementation. Internally a createAutoField was added as well in order to reduce the amount of boilerplate theme code to a minimum.

Open questions:

  • Is AutoFieldContext a good name? Maybe AutoField.context or AutoField.componentDetectorContext is a better one?
  • What about createAutoField - should we consider it an internal API for now/forever?
  • Right now overriding the default detector requires always return a component. In order to fall back to the default one, one would have to either fetch the default one with useContext or implement it in their own project. Another solution would be to treat the provided one with a higher priority and fallback to the default one.

Closes #640.


Update 2020-12-21:

  • I've decided to move AutoFieldContext to AutoField.componentDetectorContext in order to reduce API surface. We may change it in the future.
  • All createAutoField, AutoField.componentDetectorContext, and AutoField.defaultComponentDetector are considered internal for now.
  • Default detector is now accessible through AutoField.defaultComponentDetector.

@radekmie radekmie added the Type: Feature New features and feature requests label Sep 10, 2020
@radekmie radekmie added this to the v3.1 milestone Sep 10, 2020
@codecov
Copy link

codecov bot commented Sep 10, 2020

Codecov Report

Merging #800 (72fff81) into master (bbc4fcf) will decrease coverage by 0.05%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #800      +/-   ##
==========================================
- Coverage   97.54%   97.48%   -0.06%     
==========================================
  Files         175      176       +1     
  Lines        3420     3339      -81     
  Branches      658      651       -7     
==========================================
- Hits         3336     3255      -81     
  Misses          8        8              
  Partials       76       76              
Impacted Files Coverage Δ
packages/uniforms-antd/src/AutoField.tsx 100.00% <100.00%> (ø)
packages/uniforms-bootstrap3/src/AutoField.tsx 100.00% <100.00%> (ø)
packages/uniforms-bootstrap4/src/AutoField.tsx 100.00% <100.00%> (ø)
packages/uniforms-material/src/AutoField.tsx 100.00% <100.00%> (ø)
packages/uniforms-semantic/src/AutoField.tsx 100.00% <100.00%> (ø)
packages/uniforms-unstyled/src/AutoField.tsx 100.00% <100.00%> (ø)
packages/uniforms/src/createAutoField.tsx 100.00% <100.00%> (ø)
packages/uniforms/src/index.ts 100.00% <100.00%> (ø)
packages/uniforms-material/src/NestField.tsx 82.35% <0.00%> (-0.99%) ⬇️
packages/uniforms-bootstrap3/src/SelectField.tsx 88.37% <0.00%> (-0.76%) ⬇️
... and 30 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bbc4fcf...72fff81. Read the comment docs.

@Monteth
Copy link
Member

Monteth commented Sep 17, 2020

1.) AutoFieldContext seems like a good name for that.
2.) I don't see why someone would use that, maybe we shouldn't introduce a flood of API that no one will use.
3.) I would say many users may want to replace only one/very few components and they won't write the whole detector as most of its job is still correct.

@kestarumper
Copy link
Member

kestarumper commented Sep 24, 2020

  1. After some thought, AutoFieldContext seems to be the right choice. Imagine writing AutoField.context.Provider - it looks strange.
  2. I actually like createAutoField helper. It reduces boilerplate code and makes sure that we use connectField(..., { kind: 'leaf' }) for performance reasons. Most of the time, overriding AutoField (by writing it yourself) leads to reinventing the wheel. With the use of createAutoField, I can focus on selecting the right component based on its props - and that's all I need.
  3. I don't know if that's overkill, but we could export defaultComponentDetector which accepts a map of components
{
    BoolField: MyCustomBoolField,
...
}

Then if someone would only want to override (or create a new theme), one would pass overridden map and wouldn't have to change the switch logic.

If component return logic is more complicated than based on the default, one could write its own componentDetector and if needed use and fallback to our exported defaultComponentDetector.

kestarumper
kestarumper previously approved these changes Sep 24, 2020
@radekmie
Copy link
Contributor Author

@kestarumper

  1. I can imagine AutoField.context.Provider. And I think it's actually better than a new export from the theme.
  2. No, createAutoField won't add kind: 'leaf' for you - it'll only use it when it's there.
  3. The defaultComponentDetector would have to be a function already (see the allowedValues branch). There's no need for it anyway, if we'd go with default if undefined.

@kestarumper
Copy link
Member

No, createAutoField won't add kind: 'leaf' for you - it'll only use it when it's there.

I meant only that if you have your own implementation of AutoField, then if you don't do the following:

return 'options' in component && component.options?.kind === 'leaf'
      ? createElement(component.Component, props)
      : createElement(component, originalProps);

then It's lost.

@radekmie radekmie requested a review from kestarumper December 21, 2020 10:53
@radekmie radekmie marked this pull request as ready for review December 21, 2020 10:55
@radekmie
Copy link
Contributor Author

I've updated the description. If everything will be fine, I'll update all themes.

Monteth
Monteth previously approved these changes Dec 21, 2020
kestarumper
kestarumper previously approved these changes Dec 30, 2020
@radekmie radekmie dismissed stale reviews from kestarumper and Monteth via fe3c49f December 31, 2020 10:09
Monteth
Monteth previously approved these changes Jan 28, 2021
@radekmie radekmie merged commit e8d8844 into master Feb 3, 2021
@radekmie radekmie deleted the custom-autofield branch February 3, 2021 13:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Feature New features and feature requests
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

make it more easy to replace components without requiring to implement a custom AutoField
3 participants