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

#167164932 landing page #7

Merged
merged 3 commits into from
Sep 2, 2019
Merged

Conversation

allebd
Copy link
Contributor

@allebd allebd commented Aug 29, 2019

Pivotal tracker story

#167164932

What does this PR do?

User should be able to see a landing page

Summary of Task

  • Create Footer Component
  • Create other component for landing page
  • Create styling

How can this be tested?

  • Use git clone to clone this repository to local branch
  • Use npm install to download all necessary packages
  • Use npm run dev to test

Screenshots (if appropriate)

screencapture-localhost-3000-2019-08-29-13_05_09

Sample of the landing page

screencapture-localhost-3000-2019-08-29-13_07_09

Sample of landing page mobile view

@Nkemjiks
Copy link
Contributor

Remove the dummy component and route

@Nkemjiks
Copy link
Contributor

Move the membership and weekly novel component to the landing page component. Also remove the components in the folder and file name. We already know its a component because its in the component folder

@Nkemjiks
Copy link
Contributor

Change WeeklyNovel to NovelOfTheweek. That is a better name

@Nkemjiks
Copy link
Contributor

I thought there was only one without an api. I don't see any api calls here?

src/components/App.js Outdated Show resolved Hide resolved
@allebd
Copy link
Contributor Author

allebd commented Aug 29, 2019

Remove the dummy component and route

Noted, would remove the dummy component

@allebd
Copy link
Contributor Author

allebd commented Aug 29, 2019

Move the membership and weekly novel component to the landing page component. Also remove the components in the folder and file name. We already know its a component because its in the component folder

Noted, would work on it

@allebd
Copy link
Contributor Author

allebd commented Aug 29, 2019

Change WeeklyNovel to NovelOfTheweek. That is a better name

Noted, would rename the component

@allebd
Copy link
Contributor Author

allebd commented Aug 29, 2019

I thought there was only one without an api. I don't see any api calls here?

There is currently no particular api call for the landing page, but I can try and make use of the get novels api call to achieve the novel of the week

@Nkemjiks
Copy link
Contributor

You need to add support for wider screens.

@Nkemjiks
Copy link
Contributor

I thought there was only one without an api. I don't see any api calls here?

There is currently no particular api call for the landing page, but I can try and make use of the get novels api call to achieve the novel of the week

And the hero image, is that not supposed to be served by an API?

@Nkemjiks
Copy link
Contributor

And can you cherry-pick nero's navbar and make the necessary update so you page can be complete

@allebd
Copy link
Contributor Author

allebd commented Aug 29, 2019

And can you cherry-pick nero's navbar and make the necessary update so you page can be complete

Okay, would do that

@allebd
Copy link
Contributor Author

allebd commented Aug 29, 2019

You need to add support for wider screens.

Noted, would work on it

@allebd
Copy link
Contributor Author

allebd commented Aug 29, 2019

I thought there was only one without an api. I don't see any api calls here?

There is currently no particular api call for the landing page, but I can try and make use of the get novels api call to achieve the novel of the week

And the hero image, is that not supposed to be served by an API?

This can also be served from the get novels api call

@allebd allebd force-pushed the feature/167164932/landing-page branch from 1b504f2 to 0e7eb4c Compare August 29, 2019 19:50
import Navbar from './layout/Navbar';
import LandingPage from './LandingPage/LandingPage';
import Footer from './layout/Footer/Footer';
import './app.scss';

const Welcome = () => (
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you put this in a seperate component?

Copy link
Contributor Author

@allebd allebd Aug 30, 2019

Choose a reason for hiding this comment

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

Noted, Treated Accordingly

@@ -0,0 +1,95 @@
import React from 'react';
Copy link
Contributor

Choose a reason for hiding this comment

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

Change all react file to use .jsx

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Noted, Treated Accordingly

#writingCaption {
min-height:100%;
background-size:cover;
background:url(https://res.cloudinary.com/allebd/image/upload/v1567024807/dahlia/Rectangle_4.png) rgba(0,0,0,0.3);
Copy link
Contributor

Choose a reason for hiding this comment

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

Add the css under a parent class in the component so it doesn't clash with other css classes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Noted, Treated Accordingly

<h4>Quick Links</h4>
<div className="subFooter">
<ul>
<li><Link to="/">Privacy Policy</Link></li>
Copy link
Contributor

Choose a reason for hiding this comment

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

You can set the to property to# so it doesnt leave the page

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Noted, Treated Accordingly

import Materialize from 'materialize-css';

const AuthenticatedNav = () => {
useEffect(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we have the states, setStates and use effects in a separate file so that this component file on focuses on rendering the component. The file name can be AuthenticatedNav.js while this file becomes AuthenticatedNav.jsx as i requested above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Noted, Treated Accordingly

/>
<a href="#!" type="button" data-target="mobile-demo" className="sidenav-trigger">
<img src="https://img.icons8.com/ios/50/000000/menu.png" alt="menu" className="font-icon" />
{/* <i className="fas fa-bars" />/ */}
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove this if its not been 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.

Noted, Treated Accordingly

import UnauthenticatedNav from './UnauthenticatedNav';

const Navbar = () => {
useEffect(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Move to a separate js file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Noted, Treated Accordingly

src/components/layout/Navbar/index.js Show resolved Hide resolved
$color-white: #ffffff;
$color-black: #000008;

.navbar-fixed {
Copy link
Contributor

Choose a reason for hiding this comment

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

Wrap css in a parent class

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Noted, Treated Accordingly

Create Footer Component
Create other component for landing page
Create styling

[Finishes #167164932]
@allebd allebd force-pushed the feature/167164932/landing-page branch from 0e7eb4c to b6c262c Compare August 30, 2019 18:09
@allebd allebd added the Needs review when your pr needs review label Aug 31, 2019
src/components/App.jsx Show resolved Hide resolved

const LandingPage = () => (
<div>
<Navbar />
Copy link
Contributor

Choose a reason for hiding this comment

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

The navbar and footer component is general to all components so its not supposed to be here. it should be in the router file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Noted, Treated Accordingly

</div>
</section>
</div>
<Footer />
Copy link
Contributor

Choose a reason for hiding this comment

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

Move to router file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Noted, Treated Accordingly

@@ -0,0 +1,26 @@
import React from 'react';
Copy link
Contributor

Choose a reason for hiding this comment

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

Put this file in folder and add its css file with it

@@ -0,0 +1,240 @@
/*
Copy link
Contributor

Choose a reason for hiding this comment

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

Break up this scss file. See suggestion above

@Nkemjiks Nkemjiks merged commit f0c0ab5 into develop Sep 2, 2019
@Nkemjiks Nkemjiks deleted the feature/167164932/landing-page branch September 2, 2019 10:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs review when your pr needs review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants