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

Multiple typo fixes and rephrases. #176

Merged
merged 4 commits into from
Feb 8, 2020

Conversation

gilbsgilbs
Copy link
Contributor

Disclaimer: I'm not a native english speaker, nor a user of this library.

I saw some typos and sentences I had trouble to understand on this website. Therefore I spent some time to try to improve it. I think it's better, but probably still very far from perfect since I'm neither a native english speaker nor a very good writer. I just hope that I haven't made more mistakes than there previously was 🤞 . Also, I did not review the entire website because I lost patience after a while.

Lastly, I wonder why the website is that slow. In particular, the API page makes my CPU crazy. Changing the Register options radio buttons makes my Firefox lag for one whole second for no apparent reason. It looks slightly better on Chrome, but far from good actually. Overall, it gives me a bad first impression on this library (that claims to be so much faster than Formik and others).

@vercel
Copy link

vercel bot commented Feb 8, 2020

This pull request is being automatically deployed with ZEIT Now (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://zeit.co/bluebill1049/react-hook-form-website/p30xgset2
✅ Preview: https://react-hook-form-website-git-fork-gilbsgilbs-improvements.bluebill1049.now.sh

@bluebill1049
Copy link
Member

thanks for the fixes and feedbacks ❤️ i will take a look at the API page performance today.

Copy link
Member

@bluebill1049 bluebill1049 left a comment

Choose a reason for hiding this comment

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

Thanks a lot for your work. some minor feedback

{* // Another possibility, any potential props passed to <Checkbox/> will be overrided. SomeName => Checkbox *}
{/* Option 2: pass a JSX element to the Controller. */}
{/* Note that any prop passed to the element will be overriden. */}
{/* In this case, "SomeName" will be changed to "MyCheckbox". */}
Copy link
Member

Choose a reason for hiding this comment

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

👍

src/data/en/api.tsx Outdated Show resolved Hide resolved
src/data/en/api.tsx Outdated Show resolved Hide resolved
Co-Authored-By: Bill <bluebill1049@hotmail.com>
@vercel vercel bot temporarily deployed to Preview February 8, 2020 21:07 Inactive
Co-Authored-By: Bill <bluebill1049@hotmail.com>
@bluebill1049
Copy link
Member

bluebill1049 commented Feb 8, 2020

sorry about my English is driving you losing your patient :( I guess the lib itself does need some quality uplift in terms of English, so please help us and others when you have some more free time and PR is always welcome. 🙏

@bluebill1049 bluebill1049 merged commit 5f7a856 into react-hook-form:master Feb 8, 2020
@bluebill1049
Copy link
Member

Lastly, I wonder why the website is that slow. In particular, the API page makes my CPU crazy. Changing the Register options radio buttons makes my Firefox lag for one whole second for no apparent reason. It looks slightly better on Chrome, but far from good actually. Overall, it gives me a bad first impression on this library (that claims to be so much faster than Formik and others).

can you provide us with a video record of such behavior?

@gilbsgilbs
Copy link
Contributor Author

gilbsgilbs commented Feb 8, 2020

Thank you for the prompt review and merge ⚡ .

sorry about my English is driving you losing your patient :(

I'm sorry, I didn't mean to be rude at all. After reading my initial comment, I can see how it could be interpreted in an offending way and I'm deeply sorry about that. That was unfair and disrespectful on my part, especially regarding the amazing work you and other contributors have already achieved on this documentation and on the library itself. Moreover, my english is far from irreproachable, so really don't take my words as a judgement or anything like that 🙏 .

I just lost patience because it's a very repetitive work and I ended up being tired by it 😅 .

I guess the lib itself does need some quality uplift in terms of English, so please help us and others when you have some more free time and PR is always welcome.

Yeah, it could always be improved, but most importantly, I think it does the job. I may contribute more should I replace Formik with React Hook Form, but I'm undecided at this point. Formik performances are awful in my ~20 inputs form for some reason, but I find kind of cumbersome having to pass inputRef to all the inputs of the form (I use Material UI). And I'm not 100% convinced it will improve the performances that much yet.

can you provide us with a video record of such behavior?

I can try.

@bluebill1049
Copy link
Member

Thanks @gilbsgilbs for your help and understanding.

In terms of performance, I can answer this one :)

you don't need to pass innerRef instead Controller. the problem that I try to solve here is constant re-rendering, reconciler is cheaper but it becomes not so cheaper when your component trees grow. It's up to everyone which tool to use, personally i encountered an issue with it and provided with my own solution and share amongst the community. i think it's worth to give a try rather than just read what's on the doc. 👍

@gilbsgilbs
Copy link
Contributor Author

gilbsgilbs commented Feb 8, 2020

Apparently, the API page is way faster in private browsing mode, so it must be one of my extensions messing things up somewhere. However, the radio button remains very slow in private browsing mode: https://drive.google.com/file/d/16Z5AdlqTbikv_72mKcgoFM4yPZDKw6z3/view . Do you want me to open a specific issue for this?

you don't need to pass innerRef instead Controller.

Do you mean that wrapping all my inputs like this is acceptable in term of performances?

import MUITextInput from '@material-ui/core/TextInput';

export function TextInput(props) {
  return <Controller as={MUITextInput} {...props} />
}

Combined with the FormContext provider, it would provide a very similar API to what Formik currently does for me (which is very close to raw Material-UI forms actually). If so, you're right that testing out React Hook Form wouldn't take me as much time as I initially expected and I definitely might give it a try 🤔 . However, to be fair with Formik, I would have to enable the onChange mode in React Hook Form, isn't it?

@bluebill1049
Copy link
Member

bluebill1049 commented Feb 8, 2020

Apparently, the API page is way faster in private browsing mode, so it must be one of my extensions messing things up somewhere. However, the radio button remains very slow in private browsing mode: https://drive.google.com/file/d/16Z5AdlqTbikv_72mKcgoFM4yPZDKw6z3/view . Do you want me to open a specific issue for this?

Found issue, it's due to syntax highlight, I will find a way to optimize that. yes :) issue would be good.

However, to be fair with Formik, I would have to enable the onChange mode in React Hook Form, isn't it?

It's up to you in terms of when to trigger validation, you don't need to enable onChagne mode. The controller will take care of the validation mode.

however, even with onChange mode, there is still a difference. Even with onChange we only trigger when the error appeared and disappeared, no extra re-render while user typing the same valid or invalid input.

@gilbsgilbs
Copy link
Contributor Author

It's up to you in terms of when to trigger validation, you don't need to enable onChagne mode. The controller will take care of the validation mode.

Oh 🤯. That makes a lot of sense to me now. Thank you. I really like the approach after all.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants