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

Styled Navbar #21

Merged
merged 9 commits into from
Sep 11, 2020
Merged

Styled Navbar #21

merged 9 commits into from
Sep 11, 2020

Conversation

hariom1625
Copy link
Member

@hariom1625 hariom1625 commented Sep 11, 2020

Solves Issue #16

Description: Styled the Navbar as proposed in the wireframe.
PS: The cover image is being added only for illustration purpose.

Checklist

  • I've followed the Contributing guidelines provided in the repository.
  • I've made the changes which were demanded in the linked issue.
  • I've tested my code on a Chromium based browser.
  • I've tested my code on Mozilla Firefox.
  • My code gave a clean console on debugging. (no warnings/errors)

@netlify
Copy link

netlify bot commented Sep 11, 2020

Deploy preview for houseofgeeks ready!

Built with commit 4ac7fec

https://deploy-preview-21--houseofgeeks.netlify.app

@hariom1625
Copy link
Member Author

Solves Issue #16

Description: Styled the Navbar as proposed in the wireframe.

Checklist

  • I've followed the Contributing guidelines provided in the repository.
  • I've made the changes which were demanded in the linked issue.
  • I've tested my code on a Chromium based browser.
  • I've tested my code on Mozilla Firefox.
  • My code gave a clean console on debugging. (no warnings/errors)

@hariom1625 hariom1625 closed this Sep 11, 2020
@hariom1625 hariom1625 reopened this Sep 11, 2020
@hariom1625 hariom1625 mentioned this pull request Sep 11, 2020
5 tasks
Copy link
Member

@EmperorYP7 EmperorYP7 left a comment

Choose a reason for hiding this comment

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

This PR will be merged only after #20 is merged. Avoid changing Navbar.js to minimize conflicts later. The build is looking beautiful.. Good job @hariom1625 ! 🤩

@@ -0,0 +1,4 @@
.lander-img{
width:auto;
height: auto;
Copy link
Member

Choose a reason for hiding this comment

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

Do take care of the indentation.

Copy link
Member Author

@hariom1625 hariom1625 Sep 11, 2020

Choose a reason for hiding this comment

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

As I postscript 😅 the cover image was for illustration purpose so I didn't decor that code as I haven't started working on that. Will fix that :-)

function Home() {
return (
<div className="home-component">
<h1>This is the Home page.</h1>
<img className="lander-img img-fluid" src={shadow} alt="lander"/>
Copy link
Member

Choose a reason for hiding this comment

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

Using react-lazyload would help (I guess) as suggested by @ankiiitraj

Copy link
Member Author

Choose a reason for hiding this comment

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

Will make use of it from next time, it was only for visualizing the navbar with a background other than white :-)

margin-left: 20px;
padding-left: 35px;
}
Copy link
Member

Choose a reason for hiding this comment

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

Stick the closing parenthesis to the margin here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

bottom: 19px;
height: 19px;
}
Copy link
Member

Choose a reason for hiding this comment

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

Here as well

Copy link
Member Author

Choose a reason for hiding this comment

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

Done 😅

}

}
Copy link
Member

Choose a reason for hiding this comment

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

Do correct the indentation of parenthesis in the media querries. It should be clear where the querry is ending.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

Copy link
Member

@EmperorYP7 EmperorYP7 left a comment

Choose a reason for hiding this comment

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

Good to go! 🥳 Waiting for #20 to get merged. Do change the title for this PR for avoiding confusion.

@ankiiitraj
Copy link
Member

some conflicts and you're ready to go

@EmperorYP7 EmperorYP7 mentioned this pull request Sep 11, 2020
@hariom1625
Copy link
Member Author

some conflicts and you're ready to go

I have resolved the conflicts you can review the changes now :-)

@EmperorYP7 EmperorYP7 mentioned this pull request Sep 11, 2020
5 tasks
@ankiiitraj ankiiitraj mentioned this pull request Sep 11, 2020
Copy link
Member

@EmperorYP7 EmperorYP7 left a comment

Choose a reason for hiding this comment

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

LGTM! 🚀

@ankiiitraj
Copy link
Member

nice work @hariom1625

@ankiiitraj ankiiitraj merged commit 1e9a856 into houseofgeeks:front-end Sep 11, 2020
@hariom1625
Copy link
Member Author

nice work @hariom1625

Thank you sir.

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.

3 participants