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

#3 Issue Footer Component #16

Open
wants to merge 9 commits into
base: develop
Choose a base branch
from

Conversation

karshil2309
Copy link

Updated the footer code. Please check. Thank you!!!

@vercel
Copy link

vercel bot commented Oct 9, 2020

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/girlscript-blr/build-with-gsblr-raahat/mtdtr3v0o
✅ Preview: https://build-with-gsblr-raahat-git-master.girlscript-blr.vercel.app

Copy link
Collaborator

@smilegupta smilegupta left a comment

Choose a reason for hiding this comment

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

Also please prettify the code if possible! Using prettier extension!

src/App.js Outdated
Comment on lines 7 to 13
return (
<div>
<Router>
<Switch>
<Route path="/" exact component={Header}/>
<Route path="/" exact component={footer}/>
</Switch>
</Router>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Its not the correct.

  1. Routing is all about different pages!
  2. Footer Component will come below the outside Switch Tag!
  3. For more details, Refer how to do routing in this video! https://www.youtube.com/watch?v=Law7wfdg_ls&t=1778s
  4. F for footer will be capitalized

Copy link
Author

Choose a reason for hiding this comment

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

I have corrected

  1. F capital in footer component.
  2. Outside switch tag added route path with /Footer so is this correct ?

Copy link
Author

Choose a reason for hiding this comment

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

Hey @smilegupta I changed the code but am stuck in the pr. I dont know why its having every new pr while commiting the code.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@karshil2309 what exactly are facing issue!? Can you brief more about it.

Copy link
Author

Choose a reason for hiding this comment

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

@smilegupta I have done the changes but I did some blunders with git. So may be my code hasnt been updated. Can I do new PR and update it ?




function footer() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

F should be capitalised for this fuction name!

@karshil2309
Copy link
Author

@smilegupta Please review PR done the changes

Copy link
Owner

@girlscript-blr girlscript-blr left a comment

Choose a reason for hiding this comment

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

I see that the footer created is not properly aligned with the provided wireframe. Please do the needful changes.
Screenshot 2020-10-24 at 2 05 45 PM

Also we see that you have raised one more PR #31 . Either do the final changes on this or the other PR.

src/App.js Outdated Show resolved Hide resolved
Comment on lines 27 to 57
<a
href="https://www.facebook.com/"
className="facebook social"
>
<FontAwesomeIcon icon={faFacebook} size="5x" />
</a>
<a
href="https://www.linkedin.com/"
className="linkedin social"
>
<FontAwesomeIcon icon={faLinkedin} size="5x" />
</a>
<a
href="https://www.telegram.com/"
className="telegram social"
>
<FontAwesomeIcon icon={faTelegram} size="5x" />
</a>

<a
href="https://www.instagram.com/"
className="instagram social"
>
<FontAwesomeIcon icon={faInstagram} size="5x" />
</a>
<a
href="https://www.medium.com/"
className="medium social"
>
<FontAwesomeIcon icon={faMedium} size="5x" />
</a>
Copy link
Owner

Choose a reason for hiding this comment

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

Please add the following properties to the <a> tags to remove warnings:

  • target="_blank"
  • rel="noopener noreferrer"

Screenshot 2020-10-24 at 2 11 34 PM

Copy link
Author

Choose a reason for hiding this comment

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

okay will do that..

Copy link
Collaborator

@smilegupta smilegupta Oct 27, 2020

Choose a reason for hiding this comment

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

Do you need any help? You are the last guy whose PR is not got accepted?

Copy link
Author

Choose a reason for hiding this comment

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

Yes please, while adding the background color, the page is not looking as per design.

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