search

mohitrajane / neeto-ui-challange-by-mohit-rajan-e

Don't reinvent the wheel for every new Rails project. Use sane Defaults.
https://wheel-production.neetoreviewapp.net/
MIT License
0 stars 0 forks source link

Deploy application on Heroku #8

Open mohitrajane opened 1 year ago

mohitrajane commented 1 year ago

Deploying to Neeto Deploy

mohitrajane commented 1 year ago

Deployed at: https://neeto-ui-mohit.neetoreviewapp.net/

Login credentials: Username: oliver@example.com Password: welcome

@yedhink @navaneethsdk Please review

navaneethsdk commented 1 year ago

@mohitrajane Please provide the right link for the deployed application.

mohitrajane commented 1 year ago

@navaneethsdk Updated the link

navaneethsdk commented 1 year ago

@mohitrajane A few comments:

mohitrajane commented 1 year ago

@navaneethsdk

Make it a habit to put translations at the top of a component. https://github.com/mohitrajane/neeto-ui-challange-by-mohit-rajan-e/blob/9b2e15767820d6b59029149db64f38e48ba317ec/app/javascript/src/components/Dashboard/Notes/index.jsx#L24

In the React best practices book, it is mentioned that useState() should come first. However, should this rule be broken for useTranslation() since it is used in most of the React components?

Is there any specific reason for using searchTerm as an intermediate variable? Not only does it consume memory but also it can cause confusion since the state name is also searchTerm. Also please expand e as event.

No specific reason will update.

Why have you not passed the reset type on the Cancel button?

The functionality was working without it, and since we didn't need the reset functionality, I didn't include it as a parameter.

Regarding the logistics, I believe it was communicated in the Basecamp post that all PR should be assigned to reviewers after merging them. Let me know if you missed that point or if it wasn't clear enough. I'll update the doc accordingly if the document needs more clarity.

I missed this point, will be more careful.

navaneethsdk commented 1 year ago

In the React best practices book, it is mentioned that useState() should come first. However, should this rule be broken for useTranslation() since it is used in most of the React components?

I think an exception can be given for useTranslation. What if we have to use the translation during the state declaration itself? @yedhink Can we modify this section of RBB to incorporate this suggestion?

The functionality was working without it, and since we didn't need the reset functionality, I didn't include it as a parameter.

That is not a good reasoning. Please go through this section to understand why we pass in reset type in Formik form.

yedhink commented 1 year ago

Can we modify this section of RBB to incorporate this suggestion?

Yes this can be done. Please raise an issue for it in BBA. We can take it up when someone is free.