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.

Sign up for GitHub

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

Jump to bottom

Allow closing brackets on the same line #13080

Merged
merged 1 commit into from
Feb 24, 2017
Merged

Allow closing brackets on the same line #13080

merged 1 commit into from
Feb 24, 2017

Conversation

ajain17
Copy link

@ajain17 ajain17 commented Feb 2, 2017 •
edited

Pre-Submission Checklist

  • Your pull request targets the staging branch of freeCodeCamp.
  • Branch starts with either fix/, feature/, or translate/ (e.g. fix/signin-issue)
  • You have only one commit (if not, squash them into one commit).
  • All new and existing tests pass the command npm test. Use git commit --amend to amend any fixes.

Type of Change

  • Small bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds new functionality)
  • Breaking change (fix or feature that would change existing functionality)
  • Add new translation (feature adding new translations)

Checklist:

Description

@BerkeleyTrue BerkeleyTrue added the status: waiting review To be applied to PR's that are ready for QA, especially when additional review is pending. label Feb 2, 2017
@dhcodes dhcodes self-requested a review February 2, 2017 17:23
@Greenheart Greenheart self-requested a review February 2, 2017 17:40
Greenheart
Greenheart reviewed Feb 2, 2017
View reviewed changes
Copy link
Member

@Greenheart Greenheart left a comment •
edited

Choose a reason for hiding this comment

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

@ajain17 Thanks for the quick fix! 😊

.* works but I'm not sure if { might be enough for this test?
Also \n* will be covered by \s* since newlines are whitespace.

@dhcodes
Copy link
Contributor

dhcodes commented Feb 2, 2017

@Greenheart @ajain17 I was going to go in favor of changing the first test to be: assert(code.match(/\\{\\s*\\}\\);/g) to ensure no text is there. Right now the first test checks only that the last few characters of each function are removed.

dhcodes
dhcodes reviewed Feb 2, 2017
View reviewed changes
seed/challenges/03-front-end-libraries/jquery.json Outdated
@@ -421,7 +421,7 @@
"assert(!code.match(/e\"\\);/g) && !code.match(/t\"\\);/g), 'message: Delete all three of your jQuery functions from your <code>document ready function</code>.');",

This comment was marked as off-topic.

Sign in to view

@Greenheart
Copy link
Member

@dhcodes Any updates on this? I think it would be nice against @ajain17 if we could come to a conclusion 😉

Do we request further changes or can we merge this?

@ajain17
Copy link
Author

ajain17 commented Feb 8, 2017

Hi, sorry for the delay. was traveling. will do it today.

@dhcodes
Copy link
Contributor

dhcodes commented Feb 8, 2017

Kind of indifferent. Sort of depends on if you feel the part in the between the { } should be blank

Greenheart
Greenheart reviewed Feb 10, 2017
View reviewed changes
seed/challenges/03-front-end-libraries/jquery.json Outdated
@@ -418,10 +418,10 @@
"</div>"
],
"tests": [
"assert(!code.match(/e\"\\);/g) && !code.match(/t\"\\);/g), 'message: Delete all three of your jQuery functions from your <code>document ready function</code>.');",
"assert(code.match(/\\{\\s*\\}\\);/g), 'message: Delete allllll three of your jQuery functions from your <code>document ready function</code>.');",

This comment was marked as off-topic.

Sign in to view

@raisedadead
Copy link
Member

@Greenheart @dhcodes bump for a review.

@dhcodes
Copy link
Contributor

dhcodes commented Feb 24, 2017

I'm not sure this ever got updated? @ajain17

@ajain17
Copy link
Author

ajain17 commented Feb 24, 2017 •
edited

@dhcodes I updated it about 16 days back. Let me know if anymore changes are needed.

@dhcodes
Copy link
Contributor

dhcodes commented Feb 24, 2017

Okay. Camperbot was down so it was hard to tell. My bad, will test today. Thanks!

@dhcodes
Copy link
Contributor

dhcodes commented Feb 24, 2017

Tested locally. LGTM Thanks for making this fix! 👍 💯 🌮

@dhcodes dhcodes merged commit 2123427 into freeCodeCamp:staging Feb 24, 2017
@BerkeleyTrue BerkeleyTrue removed the status: waiting review To be applied to PR's that are ready for QA, especially when additional review is pending. label Feb 24, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Greenheart Greenheart Greenheart left review comments

@dhcodes dhcodes dhcodes left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@ajain17 @dhcodes @Greenheart @raisedadead @BerkeleyTrue