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(curriculum): changed test text to use should for Front End Libraries #37762

Conversation

RandellDawson
Copy link
Member

@RandellDawson RandellDawson commented Nov 15, 2019

  • 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.
  • None of my changes are plagiarized from another source without proper attribution.
  • All the files I changed are in the same world language (for example: only English changes, or only Chinese changes, etc.)
  • My changes do not use shortened URLs or affiliate links.

Related to PR #36860

This PR makes sure all challenges' tests in the Front End Libraries section use "should " or "should not".

@RandellDawson RandellDawson added scope: curriculum Lessons, Challenges, Projects and other Curricular Content in curriculum directory. status: need to test locally labels Nov 15, 2019
@gitpod-io
Copy link

gitpod-io bot commented Nov 15, 2019

RandellDawson and others added 4 commits November 19, 2019 16:12
Co-Authored-By: Tom <20648924+moT01@users.noreply.github.com>
Co-Authored-By: Tom <20648924+moT01@users.noreply.github.com>
Co-Authored-By: Tom <20648924+moT01@users.noreply.github.com>
Co-Authored-By: Tom <20648924+moT01@users.noreply.github.com>
@RandellDawson
Copy link
Member Author

@moT01 I have committed your suggested changes.

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.

🎉 LGTM 🎉

Copy link
Member

@cmccormack cmccormack left a comment

Choose a reason for hiding this comment

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

These all look good to me; however, I would have preferred to have you and your removed from most of the comments as it seems a bit personal.

For example:

- text: You should add an <code>i</code> element with the classes <code>fas</code> and <code>fa-thumbs-up</code>.

could be rewritten

- text: An <code>i</code> element with the classes <code>fas</code> and <code>fa-thumbs-up</code> should be added.

Again, this looks good to me and I approve as-is, if you agree with what I stated above I could do the work to go through them again, just let me know.

Copy link
Contributor

@Manish-Giri Manish-Giri left a comment

Choose a reason for hiding this comment

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

LGTM 💯

@RandellDawson
Copy link
Member Author

@cmccormack According to the current Writing challenge descriptions/instructions section, the challenge text should be written using the second person "you" and most of the tests in the curriculum seem to be written this way. I do not think it necessary, but you can create a separate issue where we can all discuss it I suppose. In the mean time, I think this PR can be merged now.

@ojeytonwilliams
Copy link
Contributor

I agree that there's no need to hold this up pending a change in the style guide.

@ojeytonwilliams ojeytonwilliams merged commit 6fc32ab into freeCodeCamp:master Nov 22, 2019
@RandellDawson RandellDawson deleted the fix/change-all-challenge-tests-text-to-use-should-front-end-libaries branch November 22, 2019 14:31
abbathaw pushed a commit to abbathaw/freeCodeCamp that referenced this pull request Jul 24, 2020
…ies (freeCodeCamp#37762)


Co-Authored-By: Tom <20648924+moT01@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scope: curriculum Lessons, Challenges, Projects and other Curricular Content in curriculum directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants