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(learn): Improve tests for the Nest an Anchor Element within a Paragraph challenge #41456

Conversation

RandellDawson
Copy link
Member

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.
  • All the files I changed are in the same world language, for example: only English changes, or only Chinese changes, etc.

I created this PR to address a recurring confusion I see in the Nest an Anchor Element within a Paragraph challenge. This is the first challenge where the user is introduced to the concept of nesting an existing element into a newly created element. So many times, a user ends up creating another anchor element instead of just using the existing anchor element. Creating a new anchor element without the exact attributes/text of the original can lead to problems such as the one in this forum topic.

This PR aims to better guide the user into writing the correct code. This is similar to the approach we are taking in the v7 curriculum.

@RandellDawson RandellDawson added the scope: curriculum Lessons, Challenges, Projects and other Curricular Content in curriculum directory. label Mar 12, 2021
@gitpod-io
Copy link

gitpod-io bot commented Mar 12, 2021

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.

Looks good to me.

@raisedadead raisedadead enabled auto-merge (squash) March 12, 2021 16:45
Copy link
Member

@raisedadead raisedadead left a comment

Choose a reason for hiding this comment

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

I disagree. (kidding just testing out the auto merge, randy is awesome)

@raisedadead
Copy link
Member

raisedadead commented Mar 12, 2021

@moT01 can you approve this change. I believe the merge should be held in place due to my change request review.

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 🎉

@raisedadead
Copy link
Member

Bazinga! That works

@raisedadead
Copy link
Member

@RandellDawson go ahead and dismiss my review. It should merge the PR.

@RandellDawson
Copy link
Member Author

@raisedadead Cool, so not until you either dismiss your requested change or approve it, will anything else happen?

@raisedadead
Copy link
Member

Cool, so not until you either dismiss your requested change or approve it, will anything else happen?

That is correct. Why don't you go ahead and try dismissing my review?

@RandellDawson RandellDawson dismissed raisedadead’s stale review March 12, 2021 17:09

Testing dismissal of test review

@raisedadead raisedadead merged commit c198d50 into freeCodeCamp:main Mar 12, 2021
@RandellDawson RandellDawson deleted the fix/modify-and-add-new-tests-for-nest-an-anchor-element-within-a-paragraph branch March 12, 2021 17:09
@naomi-lgbt
Copy link
Member

Woah that's neat.

@RandellDawson
Copy link
Member Author

RandellDawson commented Mar 12, 2021

I just hope this does not get abused. For instance, if there was a hold out mod that was requesting changes and two other mods approve and just decide to dismiss the other mod's requested changes. It could be a non-issue.

@ShaunSHamilton
Copy link
Member

I do not see myself making all that much use of this auto-merge feature, because I often edit the commit message.

@naomi-lgbt
Copy link
Member

@ShaunSHamilton for what it is worth, the default commit message would be the title of the pull request.

@raisedadead
Copy link
Member

because I often edit the commit message.

You will be able to edit the message prior commit. Everything works as usual (the only advantage is you skip the wait for a CI to pass, which could be helpful if we mandated the Cypress tests to be required)

Screenshot of editing commit message prior enabling auto merge:

image

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