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

fix(client): refactor formfields #39552

Merged
merged 2 commits into from
Sep 15, 2020

Conversation

ShaunSHamilton
Copy link
Member

Checklist:

  • I have read freeCodeCamp's contribution guidelines.
  • My pull request has a descriptive title (not a vague title like Update index.md)
  • My pull request targets the master branch of freeCodeCamp.
  • All the files I changed are in the same world language, for example: only English changes, or only Chinese changes, etc.

Closes #39520

@gitpod-io
Copy link

gitpod-io bot commented Sep 9, 2020

@ojeytonwilliams
Copy link
Contributor

Since we're refactoring, I think it's worth renaming things to improve clarity a bit. For instance, passing formFields to FormFields is a bit confusing to me. How about formFields -> attributes and those attributes could be{ name: 'name', label: 'label' }?

@ShaunSHamilton
Copy link
Member Author

ShaunSHamilton commented Sep 10, 2020

How about formFields -> attributes and those attributes could be{ name: 'name', label: 'label' }?

If you think this is clearer, then I will do this. It is just, name is used as many attributes (HTML), but label is not used as any attribute (HTML).

@ojeytonwilliams
Copy link
Contributor

Good point, label is clearly wrong - at the very least it should be labelText.

So, the ideal solution is probably to use a map from formFields to <FormField/> components, but that's a marginal improvement and a fair bit of work. How about just renaming field -> name and fieldLabel -> labelText? At least then we're being a little more specific.

@ShaunSHamilton
Copy link
Member Author

@ojeytonwilliams I have edited my previous comment, as I accidentally said the opposite of what I meant by the name (currently field) property.

I prefer the property of the current fieldLabel to be changed to label. However, my point was more to do with changing the name of formFields (the array of object literals) to attributes.

This is more work, and risks more confusion than not, but what if we:

  1. separate the current field from fieldLabel
  2. rename formFields -> attributes
  3. convert renamed attributes to an array containing 'solution', 'githubLink'
  4. pass a separate array labels containing the fieldLabels

@ojeytonwilliams
Copy link
Contributor

I think passing two arrays would just make things needlessly messy. What you have now seems good, but I'd go back to formFields because, as you rightly pointed out, label is not an attribute.

@ojeytonwilliams
Copy link
Contributor

Thanks for sorting that out @SKY020. There're a few minor things we can do to clean it up further, but I'll wait until Randy has taken another look. Just in case he spots something more substantial.

@RandellDawson
Copy link
Member

@SKY020 @ojeytonwilliams Looks good now. I like the fact that you got rid of the attributes prop. That was confusing.

Copy link
Contributor

@ojeytonwilliams ojeytonwilliams left a comment

Choose a reason for hiding this comment

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

Looking good! I only found a couple of minor things:

client/src/components/formHelpers/FormFields.js Outdated Show resolved Hide resolved
client/src/components/formHelpers/FormFields.js Outdated Show resolved Hide resolved
Copy link
Contributor

@ojeytonwilliams ojeytonwilliams left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Thanks, Shaun, everything is working perfectly.

Could you rebase this into two commits? One removing the redundant code and the other with the form refactor.

@ojeytonwilliams ojeytonwilliams marked this pull request as ready for review September 11, 2020 08:17
@ojeytonwilliams ojeytonwilliams requested a review from a team September 11, 2020 08:17
@ojeytonwilliams
Copy link
Contributor

Seeing as we're already reviewing it, I figured it was ready for review!

Sky020 and others added 2 commits September 11, 2020 15:14
Co-authored-by: Oliver Eyton-Williams <ojeytonwilliams@gmail.com>
@ojeytonwilliams
Copy link
Contributor

Hey Shaun, I've removed the merge commit and removed the comments from the first commit. Let me know if everything still looks good.

@ShaunSHamilton
Copy link
Member Author

Thanks, for doing that, Oliver. Everything looks ready to go.

@ojeytonwilliams
Copy link
Contributor

Cool, thanks for verifying. @ahmadabdolsaheb could you give this a quick sanity check?

@ahmaxed ahmaxed merged commit dea4e51 into freeCodeCamp:master Sep 15, 2020
@ShaunSHamilton ShaunSHamilton deleted the fix/refactor-solutionform branch July 20, 2021 20: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.

Feature to Refactor SolutionForm
4 participants