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

Issue #29 #47

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Issue #29 #47

wants to merge 3 commits into from

Conversation

cowlesrk
Copy link

@cowlesrk cowlesrk commented May 17, 2017

Fixed Issue #29 Not all of Sign In button is a Link

I surrounded the sign in button with its own form tag. This solution should also work even when JavaScript is disabled.

Not sure I did this correctly for the pull request, but fixed Issue #21 Add favicon to site

Although I originally wanted to use the svg favicon Ham created, I ran into a lot of trouble. Apparently svg files require extra code to work in React (https://css-tricks.com/creating-svg-icon-system-react/). The extra amount of code required seemed like a lot for just a favicon, so I decided to convert the svg back into an ico file. I'm not sure if this is the right thing to do in this situation or not?(http://www.creativebloq.com/web-design/icon-fonts-vs-svg-101413211). @hamholla let me know which format you think would be best for this.

@binarymason binarymason requested a review from hamholla May 18, 2017 18:19
@binarymason
Copy link
Member

pinging @hamholla

@binarymason binarymason changed the base branch from development to master May 19, 2017 00:50
@binarymason
Copy link
Member

Hi @cowlesrk I just realized that this PR is handling two issues. Do you mind breaking them out to have a separate PR for each issue? That is usually better practice anyway so things are focused.

See #51 (comment) for instructions on how to do that.

@hamholla
Copy link
Contributor

hamholla commented May 19, 2017

@cowlesrk Looks good! But, instead of using a form, lets just take the "link to" tag and apply the "btn btn-primary" classes to it. <Link to="/login" className="btn btn-primary">Sign In</Link>

@cowlesrk
Copy link
Author

cowlesrk commented May 22, 2017

@binarymason I read through the link and Tammy's FAQ, but I'm still confused at how to select and remove my commits. When I do the rebase none of my commits show up - it's a bunch of other people's:

`Rachels-MacBook-Air:client rachelcowles$ git rebase -i origin/development

pick 197ea0d updates title to CLTJRDEVS (#45)
pick 52510df add space between first and last name on leaderboard (#46)
pick ac2666b Add Heroku generated app.json
pick 7641cd1 Force HTTPS in production via buildpack
pick 95ad150 Protect against https downgrade attacks
pick 58fe6b2 Added comment count to challenge card. Resolves issue #28 (#55)
pick 9ccf2e4 Added FAQ nav link. (#53)
pick ff3e435 Added labels to "challenge" cards. Also challengeCard styles. (#56)
pick 0daf458 Removed @ from placeholder text in acct settings (#59)
pick 7e095b5 Bugfix/linkedin (#58)

@kirillian
Copy link

@cowlesrk You may benefit from pairing with someone for a little bit. Why don't you make a request in the #questions channel in slack or even in #general to pair with you.

@binarymason
Copy link
Member

@cowlesrk you mind making a shout out in #questions so we can get this merged?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants