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

About page #60

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

About page #60

wants to merge 5 commits into from

Conversation

EChesters
Copy link
Contributor

Addresses issue: #18

What this does

  • Adds banner to About page
  • Adds dummy text to About page
  • Moves nav inside container fluid

Screenshots

Desktop version of About page

Mobile version of About page

[x] Removed unused background sass file
@EChesters
Copy link
Contributor Author

Footer is broken, but this needs to be fixed separately.

@@ -13,14 +13,15 @@ export default class Layout extends React.Component {
render() {
return (
<div className="page-wrap">
<Nav />
Copy link
Member

Choose a reason for hiding this comment

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

This will be addressed in the pull request for #10. Can it be removed from this pull request pretty please?

@@ -13,7 +13,7 @@ export default class Nav extends React.Component {
});

return (
<nav className="navbar navbar-fixed-top">
<nav className="navbar navbar-default">
Copy link
Member

Choose a reason for hiding this comment

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

This will be addressed in the pull request for #10. Can it be removed from this pull request pretty please?

@@ -1,4 +1,8 @@
---
# These lines are for Jekyll
Copy link
Member

Choose a reason for hiding this comment

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

Why are these lines needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The 6 dashes are a Jekyll thing, so Jekyll knows it's has to do something with the file.

background-position: top center;
background-repeat: no-repeat;
background-size: cover;
background-color: #fff;
Copy link
Member

Choose a reason for hiding this comment

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

If we are having a background image covering the whole view, is this background-color needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed in latest commit.

@@ -0,0 +1,6 @@
@mixin background-size($size) {
Copy link
Member

Choose a reason for hiding this comment

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

Is this mixin being used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now being used in latest commit.

<div className="pt-5">
<PageHeader>{ about.title }</PageHeader>
<p>{ about.lorem }</p>

Copy link
Member

Choose a reason for hiding this comment

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

Blank line?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed in latest commit.

This was referenced Apr 18, 2017
EChesters added 2 commits May 7, 2017 00:29
- Removed blank line in About.jsx
- Used the background-size mixin
@tanyapowell
Copy link
Member

All good this end (once the conflict has been resolved) 😸

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.

2 participants