Filip Stojkov• 230
@thentrsfs
Submitted
Hello guys i did this by using react and i would like to get some review because i know its pretty messy and bad and i would like to see what mistakes did i made, thanks in advance.
Dawid "makaroni" Markieton
@greenMakaroni
All comments
@greenMakaroni
Posted
The only thing I would improve is to center the newsletter sign-up by settings its parent element to: width: 100vw; height: 100vh; display: flex; justify-content: center; align-items: center; Also, not sure that router is necessary in this project, its nice to know it, but you could just render components based on state, its better to get into a habit of not installing dependencies that you don't really need, its just extra overhead.
Your submission is very good, much better than mine. :D
Marked as helpful
1- LyttleGC• 30
@LyttleGC
Submitted
One thing I struggled with was getting the proper amount of white space surrounding the QR code image. I just eyeballed it, how would I be able to have more precise white space surrounding the image given that the image exists inside of a card?
Another question I kept having was that in the image the paragraph below the heading was a slight shade lighter but I did not see any styling in the guide that stated how to approach that. How would I fix the paragraph to better math the preview?
@greenMakaroni
Posted
As for the white space, you realistically got two options, 1st is the margin of the QR code image, and 2nd is the padding of the card. The way I'd approach this is pretty much the same, trial and error eyeballing.
When it comes to the text color, the CSS property "color" lets you set the color of the text, see the reference below: https://www.w3schools.com/css/css_text.asp
You could try different shades of gray to see which one works best, if you want an exact value however, one thing you could do is to import the sample image to paint, use colour picker tool on the gray paragraph, and then navigate to the Edit colours tool and this should give you the exact rgb values of the color you picked.
I hope that will help, overall good job!
Marked as helpful
1 - Ouss_Ach• 780
@OUSSAMAACHICHE
Submitted
*give me your suggestions `Thank you 😘
@greenMakaroni
Posted
Very nice work, I like how your buttons work, one thing you could add is a "Rate again" button on the "thank you" page that will take user back to the rating.
1 - Rohmat Setiawan• 60
@rohmatsetiawanmta
Submitted
Please let me know how I can enhance this code. Kindly consider the following key points:
- Is it accessible?
- Is it responsive?
- Is the code organized and easy to read?
- Does the layout appear appealing?"
@greenMakaroni
Posted
I think the only thing that's missing to make it pixel perfect is to increase the letter-spacing in the paragraph.
Excellent job.
Marked as helpful
1 - Tyler Gibbons• 10
@wtygibbs
Submitted
Not sure how to add a new font - Outfit was not available to me.
Was not sure how to do text placement, I settled with text-align center.
@greenMakaroni
Posted
There's many ways to add fonts to your website, these days meta is to use the google fonts CDN, You don't have to know what Content Delivery Network is in able to use it, and its super simple to use it.
Here's a google fonts website to get the fonts you need -> https://fonts.google.com/
Here's a 3min tutorial on how to install fonts to your website using google fonts -> https://www.youtube.com/watch?v=iIqgW-stZmE&ab_channel=ByteGrad
Good job and have fun learning :>
Marked as helpful
0 @yuKalashnikova2
Submitted
- How can I improve my code?
@greenMakaroni
Posted
For starters you could remove attribution div from your qr-component as it affects the qr-component layout,
simple fix would be to give the 'attribution' div the following styling: -> position: absolute;
It'll make this div not affect other elements on your website. Once its absolute, you can give it any combination of -> top: 0; -> bottom: 0; -> left: 0; -> right: 0; in order to position the attribution somewhere by the edge outside of the qr-code component.
Here is some useful references if you're interested to learn: -> https://www.w3schools.com/css/css_positioning.asp
0- Kim• 30
@TKNsss
Submitted
During the project, I struggled with the centering problem and didn't know how to make the picture fit the container. How can I improve my code in this project? Pls, give me some advice! Thanks
@greenMakaroni
Posted
If you want to center elements in a div, set that div to flex and then give it justify-content: center; align-items: center;
Say that you want to render element in the middle of your screen, then just make sure your div has the width and the height of the entire screen e.g. giving width: 100vw; and height: 100vh; and make sure to margin-0, as sometimes default margins can mess with your layout.
Here are references I'd recommend to go through. align-items -> https://developer.mozilla.org/en-US/docs/Web/CSS/align-items justify-content -> https://developer.mozilla.org/en-US/docs/Web/CSS/justify-content
if you're confused about what is vh and vw -> https://www.w3schools.com/cssref/css_units.php
good job and have fun learning :>
Marked as helpful
0
