Decompose App into components #4428
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,88 +1,15 @@ | ||
| import Header from './components/Header/Header'; | ||
| import Navigation from './components/Navigation/Navigation'; | ||
|
There was a problem hiding this comment. Inconsistent imports: There was a problem hiding this comment. Header imports Navigation from |
||
| import Welcome from './components/Welcome/Welcome'; | ||
| import Article from './components/Article/Article'; | ||
|
|
||
| const App = () => ( | ||
| <> | ||
| <Header /> | ||
| <Navigation /> | ||
|
There was a problem hiding this comment. Navigation is rendered twice - once in App.jsx (line 9) and once inside Header.jsx (line 7). Remove one of them. Based on the previous review feedback, remove the
Comment on lines
+8
to
+9
There was a problem hiding this comment. Navigation is rendered inside Header, so it should not also be rendered directly in App.jsx. This is duplicate rendering. Remove line 2 (Navigation import) and line 9 () from App.jsx. |
||
| <Welcome /> | ||
| <Article /> | ||
| </> | ||
| ); | ||
|
|
||
| export default App; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1 +1,35 @@ | ||
|
|
||
| .article { | ||
| margin: 5em auto 0; | ||
|
There was a problem hiding this comment. This file contains JSX syntax (like |
||
| padding: 1em; | ||
| font-size: 140%; | ||
| max-width: 800px; | ||
| background: white; | ||
| box-shadow: rgba(0, 0, 0, 0.05) 0 3px 15px; | ||
| } | ||
|
|
||
| .article__paragraph { | ||
| margin: 0; | ||
| color: #333; | ||
| } | ||
|
|
||
| .article__paragraph:not(:first-child) { | ||
| margin-top: 0.7em; | ||
| } | ||
|
|
||
|
|
||
| @media (min-width: 500px) { | ||
|
|
||
|
|
||
| .article { | ||
| margin: 3.5em auto 0; | ||
| padding: 2em; | ||
| } | ||
| } | ||
|
|
||
|
|
||
| @media (min-width: 800px) { | ||
| .article { | ||
| margin: 3.5em auto; | ||
| } | ||
|
Comment on lines
+21
to
+34
There was a problem hiding this comment. The
Comment on lines
+21
to
+34
There was a problem hiding this comment. The
Comment on lines
+21
to
+34
There was a problem hiding this comment. The |
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,65 @@ | ||
| // import a css file containig article styles | ||
|
There was a problem hiding this comment. There are duplicate Navigation files: |
||
| import './Article.css'; | ||
|
There was a problem hiding this comment. Header.jsx imports Navigation from |
||
| // Create an Article function returning the HTML of article block | ||
|
There was a problem hiding this comment. This file contains JSX syntax ( |
||
|
|
||
| // Add a default export statement for Article component to use it in the other files | ||
| const Article = () => ( | ||
| <article className="article"> | ||
| <h2 className="article__title">Headline</h2> | ||
| <p className="article__paragraph"> | ||
|
Comment on lines
+8
to
+9
There was a problem hiding this comment. Navigation is imported and rendered inside Header component (line 5 of Header.jsx). Including |
||
| In elementum lorem eget est euismod ornare. Phasellus sit amet | ||
| pellentesque mauris. Aliquam quis malesuada ex. Nullam eu aliquam nibh. | ||
| Mauris molestie, urna accumsan ornare semper, augue nibh posuere lorem, | ||
| vitae feugiat sem magna eget massa. Vivamus quis tincidunt dolor. Fusce | ||
| efficitur, orci non vestibulum consequat, lectus turpis bibendum odio, in | ||
| efficitur leo felis sed justo. Fusce commodo iaculis orci, quis imperdiet | ||
| urna. Sed mollis facilisis lacus non condimentum. Nunc efficitur massa non | ||
| neque elementum semper. Vestibulum lorem arcu, tincidunt in quam et, | ||
| feugiat venenatis augue. Donec sed tincidunt tellus, a facilisis magna. | ||
| Proin sit amet viverra nibh, bibendum gravida felis. Vivamus ut nunc id | ||
| mauris posuere pellentesque. Praesent tincidunt id odio id feugiat. | ||
| </p> | ||
| <p className="article__paragraph"> | ||
| In ac nisi lacus. Fusce est dolor, tincidunt ut bibendum vitae, fermentum | ||
| ac quam. Aliquam pretium tristique nibh quis iaculis. In et cursus ex, eu | ||
| aliquet ex. Proin facilisis lacus sit amet sapien ultrices, ut vehicula | ||
| arcu lobortis. Vivamus mollis ipsum ut hendrerit molestie. Morbi lacinia, | ||
| sapien eu dictum dignissim, tellus tortor congue magna, sit amet bibendum | ||
| libero nisi id massa. | ||
| </p> | ||
| <p className="article__paragraph"> | ||
| Donec arcu elit, euismod vel lobortis eu, fringilla sit amet dolor. Cras | ||
| congue, massa nec sagittis mollis, dui felis ultrices magna, tincidunt | ||
| finibus lorem quam in sem. Morbi odio turpis, pulvinar sit amet vulputate | ||
| quis, ultricies eu libero. Donec ac maximus neque, nec maximus nibh. Morbi | ||
|
Comment on lines
+21
to
+34
There was a problem hiding this comment. Navigation CSS styles should not be in Header.css. The
Comment on lines
+21
to
+34
There was a problem hiding this comment. This |
||
| rhoncus convallis urna, accumsan porta lorem hendrerit in. Cras eget nisl | ||
| dui. Morbi faucibus nisi eget ipsum semper vulputate. Mauris nec tincidunt | ||
| lectus. Aenean ac mi consequat velit dignissim consectetur. Fusce placerat | ||
| ac ipsum ac eleifend. Aenean quis faucibus ex. | ||
| </p> | ||
| <p className="article__paragraph"> | ||
| Cras egestas tempor nibh, a fermentum lorem sollicitudin non. Nulla | ||
| facilisi. In at elit id leo tristique condimentum. Donec at est nulla. | ||
| Mauris egestas magna ut laoreet pretium. Sed ultrices suscipit vestibulum. | ||
| Lorem ipsum dolor sit amet, consectetur adipiscing elit. Fusce id sapien | ||
| eros. Vivamus viverra ultricies gravida. Nam urna nibh, blandit a | ||
| vulputate at, vehicula non nulla. Aenean ut nulla leo. Praesent in | ||
| ullamcorper est. | ||
| </p> | ||
| <p className="article__paragraph"> | ||
| Pellentesque habitant morbi tristique senectus et netus et malesuada fames | ||
| ac turpis egestas. Phasellus bibendum nec arcu eu lobortis. Nam convallis | ||
| faucibus ante sed porta. Nullam ut convallis elit, quis venenatis nunc. | ||
| Curabitur sed sem eget velit condimentum rutrum in et orci. Nunc non | ||
| suscipit eros. Suspendisse porta sem vel justo commodo dictum. Aliquam | ||
| erat ligula, fringilla nec suscipit sed, porta vitae turpis. Vestibulum | ||
| rhoncus placerat nulla vitae suscipit. Curabitur consectetur ex ut odio | ||
| tristique vehicula. Ut ligula tortor, tincidunt quis sodales vitae, ornare | ||
| a turpis. Proin sit amet finibus enim. Fusce tempus a neque vitae tempor. | ||
| Aenean rutrum, libero iaculis interdum vulputate, dui eros vehicula nisi, | ||
| at interdum enim lacus eu diam. | ||
| </p> | ||
| </article> | ||
| ); | ||
|
|
||
| export default Article; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1 +1,47 @@ | ||
|
|
||
|
There was a problem hiding this comment. Navigation.js contains JSX syntax ( |
||
| .header { | ||
| width: 100%; | ||
| padding: 1em; | ||
| font-size: 140%; | ||
| position: sticky; | ||
| top: 0; | ||
| left: 0; | ||
| right: 0; | ||
|
There was a problem hiding this comment. Navigation is rendered twice: here in App.jsx AND inside Header.jsx (which imports and uses Navigation). Remove one of these - either remove |
||
| transition: opacity 0.2s ease-in-out; | ||
| text-align: center; | ||
| } | ||
|
|
||
| .header__title { | ||
| font-weight: 600; | ||
| display: inline; | ||
| margin: 0; | ||
| padding: 0; | ||
| font-size: inherit; | ||
| } | ||
|
|
||
| .navigation__link { | ||
| display: inline-block; | ||
| outline: none; | ||
| text-decoration: none; | ||
| opacity: 0.7; | ||
| padding: 0 0.5em; | ||
| color: black; | ||
| transition: opacity 0.2s ease-in-out; | ||
| } | ||
|
|
||
| .navigation__link:hover, | ||
| .navigation__link:focus { | ||
| opacity: 1; | ||
|
Comment on lines
+21
to
+34
There was a problem hiding this comment. This violates the component decomposition principle. The |
||
| } | ||
|
Comment on lines
+21
to
+35
There was a problem hiding this comment. This CSS file contains |
||
|
|
||
|
|
||
| @media (min-width: 500px) { | ||
| .header { | ||
| text-align: left; | ||
| display: flex; | ||
| justify-content: space-between; | ||
| } | ||
|
|
||
|
|
||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,18 @@ | ||
| // import a css file containig header styles | ||
|
There was a problem hiding this comment. Navigation.js contains JSX syntax ( |
||
|
|
||
| import './Header.css'; | ||
|
|
||
| import Navigation from '../Navigation/Navigation'; | ||
|
|
||
| // Create a Header function returning the HTML of header block | ||
|
|
||
| // Add a default export statement for Header component to use it in the other files | ||
|
There was a problem hiding this comment. Navigation is rendered twice: once inside Header component (Header.jsx line 14) and once directly in App.jsx line 9. This will display navigation links twice on the page. Since Navigation is already included inside Header, remove |
||
|
|
||
| const Header = () => ( | ||
| <header className="header"> | ||
| <h1 className="header__title">Site Name</h1> | ||
| <Navigation /> | ||
| </header> | ||
| ); | ||
|
|
||
|
Comment on lines
1
to
+17
There was a problem hiding this comment. Duplicate CSS file: |
||
| export default Header; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,15 @@ | ||
|
|
||
| .navigation__link { | ||
|
There was a problem hiding this comment. Header.jsx imports Navigation from There was a problem hiding this comment. After renaming Navigation.js to Navigation.jsx, update this import path to |
||
| display: inline-block; | ||
|
Comment on lines
+1
to
+3
There was a problem hiding this comment. This file contains JSX syntax ( |
||
| outline: none; | ||
| text-decoration: none; | ||
| opacity: 0.7; | ||
| padding: 0 0.5em; | ||
| color: black; | ||
| transition: opacity 0.2s ease-in-out; | ||
|
There was a problem hiding this comment. Navigation is rendered twice: once directly in App.jsx (line 9) and once inside Header.jsx (line 12). Remove one of them. Since Header already includes Navigation, remove the |
||
| } | ||
|
|
||
| .navigation__link:hover, | ||
| .navigation__link:focus { | ||
| opacity: 1; | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,17 @@ | ||
| import './Navigation.css'; | ||
|
|
||
|
There was a problem hiding this comment. Header.jsx imports Navigation from |
||
| const Navigation = () => ( | ||
|
There was a problem hiding this comment. Navigation.js contains JSX syntax ( |
||
| <nav className="navigation"> | ||
| <a className="navigation__link" href="#about"> | ||
| About | ||
| </a> | ||
| <a className="navigation__link" href="#services"> | ||
| Services | ||
|
Comment on lines
+8
to
+9
There was a problem hiding this comment. Navigation is rendered twice - once inside Header component (line 14) and once directly in App.jsx (line 9). This will cause Navigation to display twice on the page. Consider removing |
||
| </a> | ||
| <a className="navigation__link" href="#contact"> | ||
| Contact | ||
| </a> | ||
| </nav> | ||
| ); | ||
|
|
||
| export default Navigation; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1 +1,15 @@ | ||
|
|
||
|
There was a problem hiding this comment. This file contains JSX syntax ( |
||
| .welcome { | ||
|
There was a problem hiding this comment. This import path is wrong. Header.jsx is in |
||
| height: 100vh; | ||
| background: coral; | ||
| } | ||
|
|
||
| .welcome__text { | ||
| margin: 0; | ||
| font-size: 400%; | ||
|
Comment on lines
+8
to
+9
There was a problem hiding this comment. Navigation is rendered twice: here in App.jsx AND inside Header.jsx (line 12). This causes duplicate Navigation components. Since Header already includes Navigation, remove |
||
| text-align: center; | ||
| line-height: 1; | ||
| padding-top: calc(50vh - 20pt); | ||
| display: block; | ||
| font-weight: 700; | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,14 @@ | ||
| // import a css file containig welcome styles | ||
| import './Welcome.css'; | ||
|
|
||
| // Create a Welcome function returning the HTML of welcome block | ||
|
|
||
| // Add a default export statement for Welcome component to use it in the other files | ||
|
|
||
| const Welcome = () => ( | ||
| <section className="welcome"> | ||
| <span className="welcome__text">Sticky Header!</span> | ||
| </section> | ||
| ); | ||
|
|
||
| export default Welcome; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Duplicate Navigation component exists. There's one in
src/components/Header/Navigation.jsAND another insrc/components/Navigation/Navigation.js. The task requires splitting into components but creates confusion with duplicates.