finish hero section and navbar section #15
Conversation
AbduallahBarmu
requested review from
Ammar-64,
abdulrahmantaji,
Dkay-code and
khadijahawa
|
✔️ Deploy Preview for choose-wisely ready! 🔨 Explore the source changes: cb2acbf 🔍 Inspect the deploy log: https://app.netlify.com/sites/choose-wisely/deploys/612119d55607e8000782944a 😎 Browse the preview: https://deploy-preview-15--choose-wisely.netlify.app/ |
| @@ -1,102 +1,46 @@ | |||
| import React from "react"; // , { useEffect } | |||
There was a problem hiding this comment.
you'll need to pull the new changes from dev to get the latest updates there
| <div> | ||
| <Nav className="ml-auto nav__items "> | ||
| <Nav.Link href="/"> | ||
| <span>Home </span> |
There was a problem hiding this comment.
all the text should be coming from the translation file, there shouldn't be any text hard coded, like home, about, etc...
There was a problem hiding this comment.
since it will changes in the future so I removed all the text from pages
| @import "./../../style/variables"; | ||
|
|
||
| .nav__bar { | ||
| color: #ffffff !important; |
There was a problem hiding this comment.
you'll have to avoid using important, look at Zakarias code to figure it out
There was a problem hiding this comment.
since I am using bootstrap I need to use it to handle css code
I have look it at zakarias code also he use it , take a look on the FooterButtons style
src/components/NavBar/style.scss
Outdated
|
|
||
| .nav__brand { | ||
| color: $primary-color !important; | ||
| font-size: 30px !important; |
There was a problem hiding this comment.
all the units should be in rem not px
src/components/NavBar/style.scss
Outdated
| font-family: $logo-font; | ||
| } | ||
| .nav__btn { | ||
| margin-left: 4ch; |
There was a problem hiding this comment.
what is a ch, let's try to be as consistent as possible
src/containers/About/index.jsx
Outdated
| function About() { | ||
| return ( | ||
| <div> | ||
| <h2>About page </h2> |
There was a problem hiding this comment.
also here this should be coming from the translation files
src/containers/Blog/index.jsx
Outdated
| function Blog() { | ||
| return ( | ||
| <div> | ||
| <h2>Blog page </h2> |
There was a problem hiding this comment.
same here, translation files
src/containers/Home/Hero.jsx
Outdated
| <div className="hero_contant"> | ||
| <div className="hero_title"> | ||
| <h1> | ||
| The Only Place for <br /> |
There was a problem hiding this comment.
all text from translation
src/containers/Home/Hero.jsx
Outdated
| </div> | ||
| </Col> | ||
| <Col lg={7}> | ||
| <img src={hero} alt="" /> |
| @@ -1 +1,38 @@ | |||
| // hero section for homepage | |||
There was a problem hiding this comment.
hero file should be in component not containers, it should be under Hero folder with the name index.js
|
@abduallahB I have good knowledge of your code and based on that i have the same comments that Ammar mentioned in his review |
| <BrowserRouter> | ||
| <Switch> | ||
| <Route exact path="/" component={Home} /> | ||
| <Route path="/About" component={About} /> |
There was a problem hiding this comment.
The "Ressources" section is missing from the NavBar
There was a problem hiding this comment.
I did not know how I did not see it at all but now I fix it
src/containers/Home/Hero.jsx
Outdated
| <p> universities and learning approaches. </p> | ||
| </div> | ||
| <div className="hero_search"> | ||
| <input type="search" placeholder="search... " /> |
There was a problem hiding this comment.
Could you change "search" to "Search".
src/containers/Home/index.jsx
Outdated
| import Hero from "./Hero"; | ||
| import "./style.scss"; | ||
|
|
||
| function Home() { |
There was a problem hiding this comment.
could change this to arrow function to be consistent with the rest of the code. Same for the other functions
There was a problem hiding this comment.
actually I would like to do that but I cant becase I feel more comfortable with it to make a difference between componetes and methods (function ) inside the same components so if you can change yours will be better
| } | ||
| } | ||
|
|
||
| p { |
There was a problem hiding this comment.
this p will affect all the p tags in the project, you should give it a className
| <Col lg={5}> | ||
| <div className="hero_contant"> | ||
| <div className="hero_title"> | ||
| <h1> |
There was a problem hiding this comment.
all the texts should be imported form the translation files
|
|
||
| const Universities = () => { | ||
| return; | ||
| <div></div>; |
There was a problem hiding this comment.
I think if you put these in the same line of the return it'll be solved
|
PTAL |
| @@ -0,0 +1,38 @@ | |||
| import React from "react"; | |||
There was a problem hiding this comment.
the name of this file should be index.jsx
|
@abduallahB can you please solve the issues with this branch |
close #11
close #4
I have finished NavBar taske (Routing and functionality with design)
Hero sections (functionality and designing)