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: Match single quotes #17539

Merged
merged 3 commits into from
Jun 13, 2018
Merged

Fix: Match single quotes #17539

merged 3 commits into from
Jun 13, 2018

Conversation

Vimal-Raghubir
Copy link
Contributor

@Vimal-Raghubir Vimal-Raghubir commented Jun 11, 2018

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:

  • Tested changes locally.
  • Addressed currently open issue (replace XXXXX with an issue no in next line)

Closes #17508

Description

The <a href> attribute was hard coded to check only for double quoted URLs which does not cover the single quoted URLs. After checking the documentation, single quoted URLs are supported by these tags therefore I updated the regex expression to handle them as well.

@camperbot
Copy link
Contributor

@Vimal-Raghubir updated the pull request.

@Vimal-Raghubir
Copy link
Contributor Author

I am not sure how this styling is causing the linter to fail since it works on every regex checker I've used. Could a reviewer advise on this?

@scissorsneedfoodtoo
Copy link
Contributor

@Vimal-Raghubir, thank you for taking on this issue, too!

Regex is a little tricky, with some things needing to be double escaped for it to pass the linter. I'll take a look at it later when I have my laptop in front of me. Leaving this here so we can simplify your regex a little bit:

<a href=(\'\'|\"\")>

@scissorsneedfoodtoo
Copy link
Contributor

Hi @Vimal-Raghubir, sorry for the delay.

I think I figured out how to refine both tests. Currently the challenge can be passed so long as "information about batteries" is wrapped in the <a> tag. So <a>Click here for information about batteries</a> still passes. We can change the regex to strictly match the text that we want.

Also, your test wasn't passing because of the way some characters need to be escaped in the test string. You're right, your regex works in regex checkers, but the problem was that you could have mismatching single and double quotes and still pass the check.

Could you change the tests to the following?

      "tests": [
        {
          "text": "Your code should move the anchor <code>a</code> tags from around the words \"Click here\" to wrap around the words \"information about batteries\".",
          "testString": "assert($('a').text().match(/^(information about batteries)$/g), 'Your code should move the anchor <code>a</code> tags from around the words \"Click here\" to wrap around the words \"information about batteries\".');"
        },
        {
          "text": "Make sure your <code>a</code> element has a closing tag.",
          "testString": "assert(code.match(/<\\/a>/g) && code.match(/<\\/a>/g).length === code.match(/<a href=(''|\"\")>/g).length, 'Make sure your <code>a</code> element has a closing tag.');"
        }
      ],

@scissorsneedfoodtoo scissorsneedfoodtoo 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 Jun 12, 2018
@Vimal-Raghubir
Copy link
Contributor Author

Vimal-Raghubir commented Jun 12, 2018

I figured the mismatching quotes wouldn't have been handled by my regex expression. Interesting, didn't know you could use the OR operator within the regex expression. Will make the recommended changes. Thank you for your amazing support @scissorsneedfoodtoo . 👍

@camperbot
Copy link
Contributor

@Vimal-Raghubir updated the pull request.

@scissorsneedfoodtoo scissorsneedfoodtoo merged commit c0b47ae into freeCodeCamp:staging Jun 13, 2018
@scissorsneedfoodtoo
Copy link
Contributor

👍 👍 Everything LGTM! 👍 👍

Thank you for making those changes so quickly, and I'm always happy to look at your PRs. I also didn't know about the OR operator in regex and had to do some digging. I'm still not so good with it, so I'm glad we could figure out something that works.

Looking forward to your next contribution!

scissorsneedfoodtoo added a commit that referenced this pull request Jun 13, 2018
ValeraS pushed a commit to ValeraS/freeCodeCamp that referenced this pull request Oct 12, 2018
* Fix: Match single quotes

* Fix: slash direction

* Fix: Regex expression and refined tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: waiting update To be applied to PR if a maintainer/reviewer has left a feedback and follow up is needed from OP
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants