Countries list complete #8
Conversation
…rrectly. committing to have a point to roll back to
…he image based on the aspect ratio
…images, but now the cards are not the same size
|
Check the card for Nepal the description is being cut off. |
predeyo
left a comment
predeyo
left a comment
There was a problem hiding this comment.
Few things needs fixing.
| img { | ||
| width: 100%; | ||
| } |
There was a problem hiding this comment.
Put this under country or it will bleed into all the other components, like:
.Country {
img{ ...}
}
| h1 { | ||
| margin-left: 20px; | ||
| font-size: 1.1rem; | ||
| } | ||
| ul { | ||
| list-style-type: none; | ||
| margin-top: 0.5rem; | ||
| } | ||
| li { | ||
| font-size: 0.9rem; | ||
| display: flex; | ||
| margin-left: 20px; | ||
| } | ||
| strong { | ||
| font-weight: $medium; | ||
| } |
src/App.js
Outdated
| {/* {countries.map(country => ( | ||
| <Country | ||
| theme={theme} | ||
| flag={country.flag} | ||
| name={country.name} | ||
| population={country.population} | ||
| region={country.region} | ||
| capital={country.capital} | ||
| /> | ||
| ))} */} | ||
| {/* </Grid> */} | ||
| {/* </ul> */} |
There was a problem hiding this comment.
If it is commented out, why to keep it?
src/components/Country/Country.js
Outdated
| {/* <Flag | ||
| flag={props.flag} | ||
| name={props.name} | ||
| /> */} |
src/components/Flag/Flag.module.scss
Outdated
| // .Flag { | ||
| // // float: left; | ||
| // background-size: cover; | ||
| // // width: 350px; | ||
| // // width: 100%; | ||
| // // width: auto; | ||
| // height: 150px; | ||
| // width: 350px; | ||
| // // height: auto; | ||
| // } | ||
|
|
||
| .standard { | ||
| // background-size: cover; | ||
| width: 100%; | ||
| height: 100%; | ||
| // display: block; | ||
| // object-fit: cover; | ||
|
|
||
| // width: auto; | ||
| // justify-self: stretch; | ||
| // background-size: auto; | ||
| } | ||
|
|
||
| .resize { | ||
| // width: 120%; | ||
| // height: 90%; | ||
| object-fit: cover; | ||
| // width: 100%; | ||
| // height: 100%; | ||
| // background-color: red; |
src/components/Grid/Grid.module.scss
Outdated
| .Grid { | ||
| display: grid; | ||
| // grid: repeat(100px, 4) / auto-flow 120px; | ||
| // grid-template-columns: 300px 300px 300px 300px; | ||
| grid-gap: 3em; | ||
| grid-template-columns: repeat(4, 300px); | ||
| grid-auto-rows: 400px; | ||
| // grid-template-rows: initial; | ||
| // grid-template-rows: auto auto auto auto; | ||
| // height: calc(100vh - 10px); | ||
| // height: 100%; | ||
| // place-items: start stretch; | ||
| // align-content: end; |
src/global.styles.scss
Outdated
| $element-box-shadow-light: 0 5px 10px -5px #ccc, 0 -1px 10px -5px #eee; | ||
| $element-box-shadow-dark: 0 5px 10px -5px #222, 0 -1px 10px -5px #222; | ||
|
|
||
| body { |
src/global.styles.scss
Outdated
| margin: 20px; | ||
| width: 100%; |
There was a problem hiding this comment.
Putting margin on the body element lowers the header as well and on the sided it is not needed and on the bottom it should be styled on the element that is last in the page - margin not needed here.
why did you set width 100% here?
src/global.styles.scss
Outdated
| display: flex; | ||
| justify-content: center; | ||
| align-items: center; |
There was a problem hiding this comment.
Not by design - header should stretch 100% width.
It also is introducing bugs, like you now cannot do: repeat(auto-fill, 300px) on the grid as it will shrink as the flex child is not 100% which would make this page responsive.
We have lot of cards and lot of screen space, is there a reason why not to use it?
| // grid: repeat(100px, 4) / auto-flow 120px; | ||
| // grid-template-columns: 300px 300px 300px 300px; | ||
| grid-gap: 3em; | ||
| grid-template-columns: repeat(4, 300px); |
There was a problem hiding this comment.
Should be responsive, please check comments on global.styles.scss
|
Still a few comments are we ready to merge? |
4 participants
No description provided.