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 challenge test text to use the word should for Responsive Web Design #36860

Conversation

RandellDawson
Copy link
Member

@RandellDawson RandellDawson commented Sep 27, 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.

This PR has been on my to-do list for a while. After we made the change to no longer user the assert message arguments anymore and only use the text to display to the console for failing tests, I noticed several inconsistencies in how the text messages were written. Most use a should or should not, but others did not and some made it confusing to the camper whether or not they had passed or failed based on descriptions like: "Your code has an h1 element", when it really should have stated "Your code should have an h1 element.

When I queried the existing English challenges I found 763 text strings in 319 challenges that were not using the should or should not verbiage. Currently, I have only fixed 62 text strings in 27 challenges. Before I spend any more time on this, I want to first make sure this is a worthy cause. I personally believe it makes reading through all the challenges more consistent which should allow users to quickly understand what the test expects.

EDIT: This PR only deals with the Responsive Web Design section. Other PRs will fix the other sections.

@gitpod-io
Copy link

gitpod-io bot commented Sep 27, 2019

@RandellDawson RandellDawson added language: English scope: curriculum Lessons, Challenges, Projects and other Curricular Content in curriculum directory. labels Sep 27, 2019
@ojeytonwilliams
Copy link
Contributor

Messages like "Your code has an h1 element" are clearly awful, since they imply the opposite of what is actually required. I'm absolutely in favour of changing those.

Going from "Make sure your CSS rule is properly formatted with both opening and closing curly brackets" to "Your CSS rule should be properly formatted with both opening and closing curly brackets" seems, to me, to be a less critical change. Both convey what actions need to be taken. To be clear: I think it's an improvement, but I'd accept both versions.

So, my opinion is that changing the first type is definitely a worthy cause and changing the second type is still worth doing, but of lower priority.

@moT01
Copy link
Member

moT01 commented Sep 27, 2019

Yea, I like them better how you suggested @RandellDawson - making them consistent throughout all of the challenges and more understandable would speed things up for users I think - I do recall having to read some of the test text a couple times when I was going through some of the lessons to figure it out. And it sounds like it's something you want to do, so I would say go for it.

@thecodingaviator
Copy link
Contributor

I agree with what Oliver has to say

@RandellDawson RandellDawson force-pushed the fix/change-all-challenge-tests-text-to-use-should branch from b4504d3 to 6f2e994 Compare October 27, 2019 12:53
@RandellDawson RandellDawson force-pushed the fix/change-all-challenge-tests-text-to-use-should branch from 6f2e994 to 226b584 Compare November 8, 2019 18:33
@RandellDawson RandellDawson marked this pull request as ready for review November 15, 2019 17:13
@RandellDawson RandellDawson force-pushed the fix/change-all-challenge-tests-text-to-use-should branch from 226b584 to c8c96ce Compare November 15, 2019 17:17
@RandellDawson
Copy link
Member Author

@ojeytonwilliams , @moT01, @thecodingaviator This PR is ready for review. Because there are so many challenge files which will ultimately need to be reviewed, this PR is only for the Responsive Web Design section. I will create separate PRs for the other sections over the next few days.

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.

Found a few typos and things that didn't make sense. Other than that, LGTM

RandellDawson and others added 4 commits November 19, 2019 13:27
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 and others added 3 commits November 19, 2019 13:31
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 Thanks for the review. Sorry about the typos and strange wording I used on some of them. I have committed all of your suggestions.

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 🎉

Co-Authored-By: Parth Parth <34807532+thecodingaviator@users.noreply.github.com>
@RandellDawson
Copy link
Member Author

@thecodingaviator I committed your suggested change.

Copy link
Contributor

@thecodingaviator thecodingaviator left a comment

Choose a reason for hiding this comment

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

LGTM

@thecodingaviator
Copy link
Contributor

@RandellDawson do you want to wait for Kris' and Oliver's reviews too?

@RandellDawson
Copy link
Member Author

@thecodingaviator I think two reviewers is enough. I tagged the others just to get attention to the PR.

@thecodingaviator thecodingaviator merged commit 9bf3fdb into freeCodeCamp:master Nov 20, 2019
@thecodingaviator
Copy link
Contributor

Thank you for your contribution to the page! 👍
We're happy to accept these changes, and look forward to future contributions. 📝

@RandellDawson RandellDawson deleted the fix/change-all-challenge-tests-text-to-use-should branch November 20, 2019 14:46
@ojeytonwilliams
Copy link
Contributor

Sorry for being late to this. I went through it anyway, but couldn't find any problems. Looks great!

abbathaw pushed a commit to abbathaw/freeCodeCamp that referenced this pull request Jul 24, 2020
…or Responsive Web Design (freeCodeCamp#36860)

* fix: changed challenge test text to use should

* fix: changed have to be used in

Co-Authored-By: Tom <20648924+moT01@users.noreply.github.com>

* fix: reworded test verbiage

Co-Authored-By: Tom <20648924+moT01@users.noreply.github.com>

* fix: improved test verbiage

Co-Authored-By: Tom <20648924+moT01@users.noreply.github.com>

* fix: improved test verbiage

Co-Authored-By: Tom <20648924+moT01@users.noreply.github.com>

* fix: corrected typo

Co-Authored-By: Tom <20648924+moT01@users.noreply.github.com>

* fix: corrected typo

Co-Authored-By: Tom <20648924+moT01@users.noreply.github.com>

* fix: changed have the to be used in

Co-Authored-By: Tom <20648924+moT01@users.noreply.github.com>

* fix: corrected verbiage

Co-Authored-By: Parth Parth <34807532+thecodingaviator@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

4 participants