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

Create ValidatedTextField #1485

Merged
merged 4 commits into from
Aug 29, 2023

Conversation

trillium
Copy link
Member

Fixes #1482

Part of larger refactor #1481

What changes did you make and why did you make them ?

  • Create ValidatedTextField
  • Handle isEdit simpler in ValidatedTextField

Screenshots of Proposed Changes Of The Website

None, this only refactors a component

@github-actions
Copy link

Want to review this pull request? Take a look at this documentation for a step by step guide!

From your project repository, check out a new branch and test the changes.

git checkout -b Spiteless-ts.1482.validated_text_field development
git pull https://github.com/Spiteless/VRMS.git ts.1482.validated_text_field

Copy link
Member

@plang-psm plang-psm left a comment

Choose a reason for hiding this comment

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

Great job @spiteless . Overall the PR optimizes the form by reducing code, and making it versatile. I do have some requested changes I'd like to see before merging.

  • Brief comment that explains the use/need of the validated text field, description of complicated props, just so future devs have a better understanding. Something as simple as 'register - react hook forms method used for the project form' etc.
  • Rename parts/medium to be a little more descriptive. Considering we may be adding reusable drop downs, etc. maybe naming the file 'form utilities'? Or something along those lines so it is clear these are connected to the project form and their relationship to that form.

Handle isEdit without a ternary operator
Library imports first, local imports second separated by line break
@trillium trillium force-pushed the ts.1482.validated_text_field branch 3 times, most recently from d36b9fb to aa6d7e8 Compare August 28, 2023 04:44
Copy link
Contributor

@MattPereira MattPereira left a comment

Choose a reason for hiding this comment

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

@spiteless nice refactor on this one! Local testing looks good 👍

Also liked @plang-psm suggestion to add explanatory comment's to the ValidatedTextField component

The ProjectForm component is looking so much better thanks to both of you! 🎉

@plang-psm plang-psm self-requested a review August 29, 2023 16:39
Copy link
Member

@plang-psm plang-psm left a comment

Choose a reason for hiding this comment

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

Great job Trillium!

@trillium trillium merged commit c006b27 into hackforla:development Aug 29, 2023
7 of 8 checks passed
@trillium trillium deleted the ts.1482.validated_text_field branch August 29, 2023 16:40
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.

Create ValidatedTextField component
3 participants