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

Clarify btn-primary instructions #12329

Merged
merged 1 commit into from Jan 16, 2017
Merged

Clarify btn-primary instructions #12329

merged 1 commit into from Jan 16, 2017

Conversation

systimotic
Copy link
Member

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

This PR updates the instructions as suggested by @erictleung in #8417. I found this stale issue on one of my random issue dives and thought I should just submit a PR so we can close another old, stale issue.

The tests look for the first button they can find. While it's not perfect, it works fine.

@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 Jan 2, 2017
raisedadead
raisedadead requested changes Jan 3, 2017
View reviewed changes
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.

Some grammatical changes.

seed/challenges/01-front-end-development-certification/bootstrap.json
"The <code>btn-primary</code> class is the main color you'll use in your app. It is useful for highlighting actions you want your user to take.",
"Add Bootstrap's <code>btn-primary</code> class to your button.",
"Note that this button will still need the <code>btn</code> and <code>btn-block</code> classes."
"The <code>btn-primary</code> class styles app elements you apply it to in order to indicate the primary action on a page. It essentially highlights actions you want your user to take.",

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

@systimotic
Copy link
Member Author

Ugh, sorry for taking so much longer to do this 😞
So I came up with something. I'm not sure if it's perfect, but I think it should be fine. I'd like to hear what you thing.

@raisedadead
Copy link
Member

Well, I think that there should be a the (article) as in

class styles the app elements it is applied to

But, I don't want to trouble you too much... 😅! Thanks for the pull request.

@raisedadead
Copy link
Member

@erictleung please have a look, and we can merge.

@systimotic systimotic force-pushed the fix/primary-button-challenge branch 2 times, most recently from ef45690 to 2f4f878 Compare January 10, 2017 20:44
@systimotic
Copy link
Member Author

@raisedadead There's no such thing as troubling me too much 😛 I added "the"

erictleung
erictleung reviewed Jan 11, 2017
View reviewed changes
seed/challenges/01-front-end-development-certification/bootstrap.json
"assert($(\"button\").hasClass(\"btn-primary\"), 'message: Your button should have the class <code>btn-primary</code>.');",
"assert($(\"button\").hasClass(\"btn-block\") && $(\"button\").hasClass(\"btn\"), 'message: Your button should still have the <code>btn</code> and <code>btn-block</code> classes.');",
"assert($(\"button\").first().hasClass(\"btn-primary\"), 'message: Your <code>Like</code> button should have the class <code>btn-primary</code>.');",
"assert($(\"button\").first().hasClass(\"btn-block\") && $(\"button\").hasClass(\"btn\"), 'message: Your <code>Like</code> button should still have the <code>btn</code> and <code>btn-block</code> classes.');",

This comment was marked as off-topic.

Sign in to view

@erictleung
Copy link
Member

@raisedadead @systimotic LTGM! I just have that one comment on a line. It's not that big of a deal, but I noticed it was missed. Otherwise, everything is good to me! 🎉

@systimotic
Copy link
Member Author

@erictleung Whoops, I missed that one! Yeah, it's very unlikely that a camper will leave one of the classes on the button and remove the other, but the tests should work no matter what the camper does. I updated the PR.

@systimotic
Copy link
Member Author

@raisedadead @erictleung Looks like this is ready to be merged!

@raisedadead
Copy link
Member

✨ ✨ LGTM. ✨ ✨

Thanks @systimotic !

@raisedadead raisedadead merged commit 9d438d2 into freeCodeCamp:staging Jan 16, 2017
@raisedadead raisedadead removed the status: waiting review To be applied to PR's that are ready for QA, especially when additional review is pending. label Jan 16, 2017
@systimotic systimotic deleted the fix/primary-button-challenge branch January 16, 2017 18:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@erictleung erictleung erictleung left review comments

@raisedadead raisedadead raisedadead approved these changes

@QuekCheeHow QuekCheeHow QuekCheeHow approved these changes

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
@systimotic @raisedadead @erictleung @QuekCheeHow @BerkeleyTrue