@Desha-SaeedSubmitted over 2 years ago
This is my solution for this challenge, I'm open for reviews of any type or best practices recommendations
John_diddles
@JohndiddlesAll comments
- @JohndiddlesPosted over 2 years ago
Hi Desha,
Really great work on this challenge.
You can look into the line-heights for your texts and also, you can add transition {say 0.2s} to the .icon-circle class to make the transition between the hover and normal state a bit more interesting.
Also for your colors, you can look into using rgba(). This can be a lot interesting and I'm sure you'll love it.
These aren't major things but you can always consider them. Still, really great job on this challenge. I love your solution.
1 - @jesuisbienbienSubmitted over 2 years ago
I'm getting more comfortable using grid. Also I spent time updating the readme.md file and I'm proud of it. I'd love to receive feedbacks on anything.
Some specific questions I have are: 1- any other way to make this design work? using grid or not using grid? 2- what would you do differently for the box-shadow, that makes it look closer to the design? 3- how do you like the active state that I added for the cards? :D
Happy coding!
@JohndiddlesPosted over 2 years agoHi Nguyen, this is nice. I like it.
about your questions, I'll probably go with grid because it'd be easier for me to move things around with grid. But you can sure get this same task done using flexbox. My solution to this task was done with flex and it was very is easy getting it done. Secondly, like Marvin said, you can add some transition effect to the hover effect.
But still, nice work. Keep coding......cheers 🍻 🥂
1 - @OussamabennabiSubmitted over 2 years ago
i really enjoyed building this project from scratch . BUT i found a problem that i didn't know how extract languages ,currencies and borders countries so i didn't implement it also this my first project ever using react so i think i did pretty well i guess.
@JohndiddlesPosted over 2 years agoHi Oussama! A very solid job you did here being your first react app.
About your questions, I have made some modifications to your detail component to fix the challenges you're having. I'll paste code here so you can compare it with yours and hopefully you can understand the fixes I made.
import { useEffect, useState } from "react"; import { Link, useParams } from "react-router-dom"; export default function Details(props) { const { darkMode } = props; const addDarkMode = darkMode ? "darkMode" : ""; const { countrieName } = useParams(); const [countrie, setCountrie] = useState([]); const [allCountries, setAllCountries] = useState(null); const [allBorders, setAllBorders] = useState(null); const fetchData = async () => { fetch(`https://restcountries.com/v2/name/${countrieName}`) .then((response) => response.json()) .then((data) => setCountrie(data[0])); }; useEffect(() => { fetch("https://restcountries.com/v2/all") .then((response) => response.json()) .then((data) => setAllCountries(data)); }, [countrie]); const { name, nativeName, population, region, subregion, capital, flag, topLevelDomain, borders, languages, currencies, } = countrie; useEffect(() => { let borderCountry = []; if (borders) { for (let i = 0; i < borders.length; i++) { allCountries.filter((country) => country.alpha3Code === borders[i] ? borderCountry.push(country.name) : "" ); } } setAllBorders(borderCountry); }, [allCountries]); useEffect(() => { fetchData(); }, [countrieName]); console.log(countrie); return ( <div className={`details ${addDarkMode}`}> <div className="container countrie-container"> <Link to="/"> <button className="btn home-btn"> <i className="fa-solid fa-arrow-left"></i> Back </button> </Link> <div className="countrie"> <div className="countrie-img"> <img src={flag} alt={name} /> </div> <div className="countrie-details"> <h2>{name}</h2> <div className="countrie-description"> <p> <span className="text">Native Name </span>: {nativeName} </p> <p> <span className="text">Population </span>: {population} </p> <p> <span className="text">Region </span>: {region} </p> <p> <span className="text">Sub Regions </span>: {subregion} </p> <p> <span className="text">Capital </span>: {capital} </p> <p> <span className="text">Top Level Domain </span>:{" "} {topLevelDomain} </p> <p> <span className="text">Currencies </span>: {currencies ? currencies.map((currency) => ` ${currency.name} `) : "loading currencies"} </p> <p> <span>Languages</span>: {languages ? languages.map((language) => " " + language.name + " ") : "loading languages"} </p> </div> <div className="border-countries"> <span className="text">Border Countries :</span> {!allBorders ? ( <div>Loading border countries...</div> ) : ( allBorders.map((country) => ( <Link key={country} to={`../detail/${country}`}> <button className="btn">{country}</button> </Link> )) )} {/* <Link to="ou"> <button className="btn">France</button> </Link> <Link to="ou"> <button className="btn">France</button> </Link> <Link to="ou"> <button className="btn">France</button> </Link> */} </div> </div> </div> </div> </div> ); }if you have any questions, please feel free to reply below, I'll be glad to jump back in here. Again, really solid job you did. Keep coding! cheers 🍻 🥂
Marked as helpful1 - @codingwithrihaSubmitted over 2 years ago
I would like your sincere feedback.
@JohndiddlesPosted over 2 years agoHi Riha! I checked back and saw that you've fixed your live link. That's great.
Your solution is nicely done. I saw that you used margins to give your flex items some spaces between them. That works. But a better practice is to give the containing div with the display of flex a gap attribute. something like this;
.container { display: flex; gap: 2em; }this separates all of the flex items by 2em both horizontally and vertically.
You can also consider using relative units such as 'em', 'rem' instead of px for your sizes especially your fonts.
Every other thing looks great. Good job! Cheers 🍻
0 - @codingwithrihaSubmitted over 2 years ago
I would like your sincere feedback.
@JohndiddlesPosted over 2 years agoThe link to the github code now works but the live link shows the content of your gh-pages branch. I think you have to go to your settings on this repo and click on pages, then change the source to main (which is the branch that has your code) and then click save. I think this should fix it.
Marked as helpful0 - @elshoraSubmitted over 2 years ago
I did well here? I made it using grid property and it worked very well.
I will be happy if I get more reviews.
@JohndiddlesPosted over 2 years agoHi Mahmoud! I really love your solution. You did really good with this.
I see you gave your grid items some margin to space out the items, it is a better practice to use gap instead of margin just like this;
.container { display: grid; gap: 20px; }This will evenly space out all the items in the grid by 20px both vertically and horizontally and you won't be needing the margins.
Also you can consider using other size units (such as em) for your texts instead of 'px'. I also see you have the same text color for all your p tags, but I think according to the designs, the colors are meant to be different. The white backgrounds have darker texts while the other ones have much lighter texts.
Overall, you did a solid solid job. Cheers 🍻
Marked as helpful1 - @codingwithrihaSubmitted over 2 years ago
I would like your sincere feedback.
@JohndiddlesPosted over 2 years agoHi Riha, how're you doing?
It seems your links are broken or you probably made the github repo private or the repo is deleted. You might want to check that out.
0 - @pablo-taubeSubmitted over 2 years ago
This project was stimulating to build because I realized, during the process, that sometimes we need to take a break, breath, and restart. I had to rewrite the CSS from the beginning because, at some point, I was not understanding what I was doing and why it was not working the way it was supposed to. And I'm glad I did it. When rewriting the code, I decided to better organize and create sections to separate what I was styling, and by doing that, I could understand the code and the results.
Even with this situation, I found myself excited and determined to find a solution.
The final result (so far) is not perfect, but I am content with the progress I've made.
@JohndiddlesPosted over 2 years agoHi Pablo, really nice job you did here.
I see you gave your profile picture a left value of 51% and I think it should be 50% so the picture is perfectly centered horizontally.
If I was to approach this challenge, I'll probably seperate the user's details and the stats into their separate parent divs. For the user's name and age, they will be in a div with display of flex. This is to make centering the texts a lot easy.
Since I have once attempted this challenge before, I'll just link you to my solution. Hopefully you can find some useful tips there. it's here
All the same, really good job you did. Cheers 🍻 🥂
Marked as helpful1 - @sadykovaSubmitted over 2 years ago
Hi fellow coders,
This is my solution to the QR component challenge:) Looking forward to your feedback!
@JohndiddlesPosted over 2 years agoReally good job Almira!
just a couple of things I want to point out.
- Typography: I think you can pay some more attention to the text font sizes.
- you can consider specifying a particular width for your texts so that they are consistent across all widths. Personally, I use
chas the unit when specifying widths for my texts. You can check out other units also. - for this particular challenge, you need to have a uniform padding across all axis. I see you used 15px for top and bottom and 5px for left and right but you can keep it at 15px on both axis.
- You can also consider using a border-radius twice the border radius of the QR code.
This are just my opinions, definitely not the only way to go about it and maybe not even the best but I think this tips could help improve your solution. overall, nice work you've done. Cheers 🍻 🥂
Marked as helpful0 - @IngridsSilveiraSubmitted over 2 years ago
Aceito dicas e feedbacks!!
@JohndiddlesPosted over 2 years agobody { background-image: url(images/pattern-background-desktop.svg); background-position: top; background-repeat: no-repeat; background-size: contain; background-color: hsl(225, 100%, 94%); width: 100vw; height: 100vh; box-sizing: border-box; display: flex; align-items: center; justify-content: center; } main { } .container { display: flex; flex-direction: column; justify-content: center; align-items: center; } .card { display: flex; flex-direction: column; align-items: center; justify-content: center; width: 350px; background-color: white; border-end-end-radius: 20px; border-end-start-radius: 20px; border-top-left-radius: 20px; border-top-right-radius: 20px; } .card-texto h1 { font-family: 'Red Hat Display', sans-serif; color: hsl(223, 47%, 23%); margin-left: 65px; padding: 8px; font-weight: 900; font-size: 24px; } .imagem { }Hi Ingrid! Good job you've done here.
Above are a few tweaks to your css I think you might find helpful. First, to center the card within the body element both horizontally and vertically, I used flex but I had to first set the width to 100vw and height to 100vh. I also took out the styles for the
<main>element. I also took out the height for the card since I want the card to take up the height of its content and not a predefined height. I also took out the margin-top for the h1 since it won't be needed anymore. For the image, I realized you used position: absolute. Although I'm not sure why you took this approach but I feel you won't need it. so I removed it and voila, it still works.Sorry about the epistle, I just like to explain whatever code it is I write so you can understand it well. But then, I think you did a really good job. Keep coding....cheers 🍻 🥂
Marked as helpful1 - @darwinvino08Submitted over 2 years ago
Hi, as i finish the ultimate html and css course of Mosh Hamedani. i want to practice what i learn so i first create this easy challenge then will go next level next. I did this by mobile first approach but as you can see it has a problem in the breakpoint layout which the layout becomes ugly and i don't know how to fix it i also have problem applying a border radius to the container. I also practiced and tried how to write a maintanable and reusable css which use object-oriented and bem . any suggestions will help me a lot to improve.. Thank you
@JohndiddlesPosted over 2 years agoHi Darwin! Really Good Job.
I feel you can switch to mobile view at an earlier point, say 999px to avoid the items from squeezing and having different heights before eventually entering into the mobile view @768px....you can also give the first item border-top-left-radius and border-bottom-left-radius while you give the last card border-top-right-radius and border-bottom-right-radius.
otherwise, you can have another
<div>inside the<section>to contain all the cards. Then you can give all the cards a height of 100% so that when a card's height grows when screen width is being adjusted, all the other cards grow accordingly and everything looks uniform all the way. If you take this approach, you can give the containing div a border radius and an overflow: hidden.I hope you find this helpful but if you need more clarification, you can drop a reply to this comment. I'd be glad to hear from you. Stay safe and happy always. Happy coding 🍻
Marked as helpful1 - @marcelx-silvaSubmitted over 2 years ago
What did you find difficult while building the project?
- Positioning the background image in mobile devices (I didnt it because the two images even not showing in the background)
Do you have any questions about best practices?
- No, I dont. But if there are areas of my code that could be better ( why ) I would like to know.
@JohndiddlesPosted over 2 years agoHi Thiago! Really nice job 👍
To center the entire card in the middle of the page, you can add the following to the body element
height: 100vh; width: 100vw; display: flex; flex-direction: column; justify-content: center; align-items: center;This will help you center the card both horizontally and vertically. Although, you'd have to adjust the position of your background images.
Also to center your image. You can bring your margins into just one line like this
.profile-picture{ z-index: 1; display: block; border-radius: 50%; border: 3px solid #fff; margin: -50px auto 5% auto; }using a value of auto on the x axis helps you center your item horizontally and merging all the margins into one line helps you keep your code a bit cleaner.
Regardless, this was a solid attempt. cheers 🍻 🥂 Keep coding...
Marked as helpful1
