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

[docs] Document mui-rff #17943

Merged
merged 2 commits into from
Oct 22, 2019
Merged

[docs] Document mui-rff #17943

merged 2 commits into from
Oct 22, 2019

Conversation

lookfirst
Copy link
Contributor

FFMUI is outdated, untested.
MUI-RFF is modern and jest tested.

Happy to keep FFMUI in there, but it just seems weird to recommend it at this point given that it isn't even based on MUI4.

Thanks for making a great project.

FFMUI is outdated, untested.
MUI-RFF is modern and jest tested.

Happy to keep FFMUI in there, but it just seems weird to recommend it at this point given that it isn't even based on MUI4.

Thanks for making a great project.
@mui-pr-bot
Copy link

mui-pr-bot commented Oct 20, 2019

No bundle size changes comparing 1e4d2db...5cee893

Generated by 🚫 dangerJS against 5cee893

@eps1lon eps1lon added the docs Improvements or additions to the documentation label Oct 20, 2019
Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

Thanks for pushing the form integration forward. I think that we should update the official demo https://codesandbox.io/s/9ywq085k9w too. Could you have a look at it?

It will help with #15585

@oliviertassinari oliviertassinari changed the title [docs] replace final-form-material-ui with mui-rff [docs] Replace final-form-material-ui with mui-rff Oct 20, 2019
@eps1lon
Copy link
Member

eps1lon commented Oct 20, 2019

Just want to clarify why I approved: It's mainly about dropping final-form-material-ui because it's not compatible with v4 and the maintainer hasn't even responded to this issue which was opened in july.

@oliviertassinari
Copy link
Member

oliviertassinari commented Oct 20, 2019

Same motivation, we need a package that is actively maintained 👍

@lookfirst I have spent some time to look at your package. I have found the following blockers:

@oliviertassinari oliviertassinari added the PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI label Oct 20, 2019
@lookfirst
Copy link
Contributor Author

lookfirst commented Oct 21, 2019

Thanks for the feedback. I recognize my stuff is definitely early. Hoping to get some project visibility, so that I can attract some help to work on it.

  • Yes on lodash. I'll work on it. Edit: Note that the dep on lodash is only there if you use that file.... that file is optional and not used anywhere in the components themselves. Just a helper if you want it.
  • On API compatibility. The issue for me with FFMUI is that it requires you to write <Field> wrappers everywhere and I'd prefer to include it within the API. I want a single component that I can include in my pages, with sane defaults, that does everything. Part of the reason for that is that figuring out all the typescript types and properties to pass, is a pain.
  • The example and tests is documentation, for now. PR's to write more documentation are welcome. Otherwise, I'll get there when I have some time.
  • Sure, I will look into updating the codesandboxes when I have some time.
  • I am still working now on standardizing and simplifying the error handling a bit more so things might change there as well.

Thanks for the feedback. WIP.

@lookfirst
Copy link
Contributor Author

lookfirst commented Oct 21, 2019

Edit, nevermind! I figured out something cool.... code isn't up yet, but I'm happy with it... you'll see... =)

@oliviertassinari
Copy link
Member

oliviertassinari commented Oct 21, 2019

@lookfirst Thank you for taking the time to consider my feedback. At the end of the day, the community will decide what works best for them.

Considering that if we had to focus on third-party form integrations, we would start with the most important library: formik. I propose that, as the core team, we optimize for the solution that requires us the less time involvement: I propose that we link final-form-material-ui and mui-rff in the documentation. We should be able to let organic growth tells us, in the next few months, which of the two libraries to retain, as ultimately, we should aim for a single encouraged adapter (per form libraries).

@lookfirst Does that work for you?

@lookfirst
Copy link
Contributor Author

I will note that while formik is more 'popular', my prior experience with it when evaluating frameworks was a bit disappointing. RFF is pretty well done.

(...talk about formik removed) ... then you say this...

I propose that we link final-form-material-ui and mui-rff in the documentation.

Hmm... not sure I understand the connection to formik in this regard. Care to explain? I'm totally fine including both, but be aware that FFMUI is effectively abandoned, so I think that should be noted so that people don't waste their time (like I did).

FYI, I'm working on a new codesandbox based on your original FFMUI one right now, which is helping me find a couple bugs and see what else needs to be implemented. It is coming along nicely and I should have it done in a few more hours. I'll update here when done.

@lookfirst
Copy link
Contributor Author

lookfirst commented Oct 21, 2019

@oliviertassinari

After a huge amount of work today... I have a codesandbox for you... they now both render exactly the same way.

Mine: https://codesandbox.io/s/react-final-form-material-ui-example-tqv09
FFMUI: https://codesandbox.io/s/9ywq085k9w

Sorry, I can't figure out how to get a shorter url for my codesandbox. =(

I've fixed all the dependencies, removed lodash and datefuns and figured out how to handle errors uniformly. The only difference is the way the components are used, which I'd argue is a lot simpler now since it doesn't require learning both API's just to get started.

Todo is more example documentation on the readme, but between the sample codesandbox and example website, I'd say things are pretty well covered now.

@oliviertassinari
Copy link
Member

Well done! I have included formik in the discussion so we don't forget what our priorities should be. To be clear on the "one adapter", it's one adapter perform library. I think that we should link both so we can let the developers decides what works best for them. I have abandoned projects for years and yet, some people still find value in them. To be honest, I don't think that these links are than much important. I expect most people to run a search query on Google before going with one option.

@oliviertassinari oliviertassinari removed the PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI label Oct 21, 2019
@oliviertassinari oliviertassinari changed the title [docs] Replace final-form-material-ui with mui-rff [docs] Document mui-rff Oct 21, 2019
@lookfirst
Copy link
Contributor Author

To be honest, I don't think that these links are than much important.

This page is how I found out about RFF. I'm really well versed in finding projects too. Formik dominates the top, but it is far from the best. RFF solves a lot of the rendering performance and typescript problems that formik has.

But 🤷‍♂ up to you.... I'm fine either way. Just glad you're including my stuff. Thanks.

@lookfirst
Copy link
Contributor Author

Also note that FFMUI is getting around 7.5k downloads / week on npm. I know those numbers are pretty inflated (my project is showing a couple hundred and nobody knows about it), but at those numbers, people must be using it... providing something that is more modern and tested, is a good thing.

@lookfirst
Copy link
Contributor Author

I just noticed the LOC of our codesanboxes...

Original is 327 and mine is 201. Not a bad reduction!

@lookfirst
Copy link
Contributor Author

@oliviertassinari Ok, I added api docs to the README. So that completes all of the things that you requested *! =)

  • Sorry, not going to do the API compatibility... just can't happen...

@oliviertassinari
Copy link
Member

@lookfirst Thank you so much for caring about this form integration issue. I think that the community would benefit from using form libraries more often.

@oliviertassinari oliviertassinari merged commit 9174b2b into mui:master Oct 22, 2019
@oliviertassinari
Copy link
Member

I have created a reminder to have a look back at this topic in 3 months.

@lookfirst lookfirst deleted the patch-4 branch October 22, 2019 10:47
@lookfirst
Copy link
Contributor Author

@oliviertassinari Thank you.

@lookfirst
Copy link
Contributor Author

@oliviertassinari I updated the demo to highlight one real performance benefit of RFF... it is quite telling to see the render count in realtime... if you're ever thinking of using formik, make sure it doesn't fail the render test... it is imho, a much better solution than formik's fastfield...

https://lookfirst.github.io/mui-rff/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Improvements or additions to the documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants