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

feat(curriculum): add registration form practice project #42876

Merged
merged 24 commits into from
Oct 14, 2021

Conversation

ShaunSHamilton
Copy link
Member

@ShaunSHamilton ShaunSHamilton commented Jul 15, 2021

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 main branch of freeCodeCamp.
  • I have tested these changes either locally on my machine, or GitPod.

Adds the Registration Form practice project to the upcoming curriculum.

  • Merge PR fixing ERM on boundary issue

@ShaunSHamilton ShaunSHamilton requested a review from a team as a code owner July 15, 2021 17:56
@gitpod-io
Copy link

gitpod-io bot commented Jul 15, 2021

@ShaunSHamilton ShaunSHamilton marked this pull request as draft July 15, 2021 17:56
@github-actions github-actions bot added platform: learn UI side of the client application that needs familiarity with React, Gatsby etc. scope: curriculum Lessons, Challenges, Projects and other Curricular Content in curriculum directory. scope: i18n language translation/internationalization. Often combined with language type label labels Jul 15, 2021
@github-actions github-actions bot added the scope: tools/scripts Scripts for supporting dev work, generating config and build artifacts, etc. label Jul 27, 2021
@ShaunSHamilton ShaunSHamilton marked this pull request as ready for review September 15, 2021 15:37
@ShaunSHamilton
Copy link
Member Author

I spent so long thinking this PR was already merged 😅

Copy link
Member

@gikf gikf left a comment

Choose a reason for hiding this comment

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

Findings from completing project once.

More general note - there doesn't seem to be part with adding value attribute to the radio and checkbox inputs

Some specific things below.

Copy link
Member

@moT01 moT01 left a comment

Choose a reason for hiding this comment

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

Overall, this looks real good @ShaunSHamilton 🎉

I left a number of suggestions - Many could be considered a preference and aren't necessary to add, others might just be thoughts or observations. Looks like @gikf left some suggestions for a number of the tests I was struggling with.

Some overall thoughts:
Seemed like some of the lessons could have maybe been broken up a little more. At the start I add 4 label elements at once - then 4 inputs at once, one in each - and 4 types... it's kind of a lot - I'm not saying it's wrong or needs to be changed - maybe something to keep in mind for the future. One way I might have done it is to do a single element, then the next three - for instance, 1 label in a step, then 1 input, then 1 type - followed by the last 3 labels, inputs, types... per step. Hope that makes sense.

There were some that could maybe have used more explanation - that's tough, because you don't want to get too wordy. For instance though, when a new element is introduced, say the radio buttons, you aren't really told what they do - for the most part, the elements are pretty self explanatory so it's not really that big of a deal - and as you build the project, you kind of just figure it out.

Another thing I would maybe like to see is some like "try it in the preview" sentences to try and get users more engaged. For instance, maybe when it mentioned that password inputs are hidden it could say something like "try it in the preview to make sure the input is hidden" - or when you set the min width of something: "resize the preview to make sure it works" - perhaps we don't want any of that, I'm not sure - but I think it adds some engagement

One frustrating thing I may have mentioned on another review is that when adding something I want to see in the preview - specifically, when I was styling the submit button - I want to see it - but every time I add a property, the preview refreshes and sends me to the top of the page. So I scroll down, and add another property, and it sends me to the top again - not much we can do about that I don't think. And I suppose that's the same as the current curriculum preview.

I don't think of the above needs to be changed here - just sharing my thoughts - maybe something to keep in mind.

Like I said - this looks real good 👍 You can tell how much thought went into it.

ShaunSHamilton and others added 2 commits September 24, 2021 13:45
Co-authored-by: Tom <20648924+moT01@users.noreply.github.com>
Co-authored-by: gikf <60067306+gikf@users.noreply.github.com>
Co-authored-by: Tom <20648924+moT01@users.noreply.github.com>
@ShaunSHamilton ShaunSHamilton added the status: waiting update To be applied to PR if a maintainer/reviewer has left a feedback and follow up is needed from OP label Sep 24, 2021
@ShaunSHamilton ShaunSHamilton added status: waiting review To be applied to PR's that are ready for QA, especially when additional review is pending. and removed status: waiting update To be applied to PR if a maintainer/reviewer has left a feedback and follow up is needed from OP labels Oct 6, 2021
@ShaunSHamilton ShaunSHamilton requested a review from moT01 October 6, 2021 13:41
@naomi-lgbt
Copy link
Member

image

I can't approve this 😆

Copy link
Member

@naomi-lgbt naomi-lgbt left a comment

Choose a reason for hiding this comment

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

This is coming along nicely.

I have a couple of tiny nitpicks, but the bulk of this is ready to go.

Co-authored-by: Nicholas Carrigan (he/him) <nhcarrigan@gmail.com>
Copy link
Member

@moT01 moT01 left a comment

Choose a reason for hiding this comment

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

Still LGTM 🎉

Edit: I can approve this: use clear, Tom-like language for 038

@ShaunSHamilton ShaunSHamilton mentioned this pull request Oct 9, 2021
4 tasks
Copy link
Member

@naomi-lgbt naomi-lgbt left a comment

Choose a reason for hiding this comment

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

I'm happy with this. Nice work Shaun!

@naomi-lgbt naomi-lgbt merged commit 4a605c5 into freeCodeCamp:main Oct 14, 2021
@ShaunSHamilton ShaunSHamilton deleted the feat/reg-form branch October 14, 2021 22:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
platform: learn UI side of the client application that needs familiarity with React, Gatsby etc. scope: curriculum Lessons, Challenges, Projects and other Curricular Content in curriculum directory. scope: i18n language translation/internationalization. Often combined with language type label scope: tools/scripts Scripts for supporting dev work, generating config and build artifacts, etc. status: waiting review To be applied to PR's that are ready for QA, especially when additional review is pending.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants