change react components to pure HTML #11
Conversation
✅ Deploy Preview for cyf-labs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
@SallyMcGrath I would appreciate, if you could review it and give me ideas on how to improve. |
SallyMcGrath
left a comment
SallyMcGrath
left a comment
There was a problem hiding this comment.
Thanks for putting this together. ⭐
If you're interested in polishing this up, you might find this workshop useful. You could ask other people to join you and do it in class or midweek.
https://github.com/CodeYourFuture/CYF-Workshops/tree/main/polish
It goes over some of the most common ways to elevate a site with professional techniques.
I would also like to see a container on the line length please! https://baymard.com/blog/line-length-readability
| } | ||
|
|
||
| .App .cyf .about-trainees figure { | ||
| .cyf .about-trainees figure { |
There was a problem hiding this comment.
Do you need this to be so specfied?
Always use the lightest weight you can when writing CSS. Only use what you must. I think many of these rules could be simplified in this way. What do you think?
https://css-tricks.com/strategies-keeping-css-specificity-low/
| body { | ||
| background-color: var(--gray); | ||
| min-height: 100vh; | ||
| min-width: 300px; |
There was a problem hiding this comment.
| min-width: 300px; |
What is this doing? Can you talk me through it?
| margin: 0; | ||
| padding: 0; | ||
| box-sizing: border-box; | ||
| font-family: "Raleway", "Roboto", "Open Sans", sans-serif; |
There was a problem hiding this comment.
Do you need to use universal selector to apply a general font rule?
Would it be more usual to apply this to the body? Have a think.
| border-radius: 5px; | ||
| margin-top: 20px; | ||
| padding: 20px; | ||
| margin-top: 1rem; |
There was a problem hiding this comment.
| margin-top: 1rem; |
Avoid using margin-top if you can. In general, top margins aren't reliable in CSS and in a more complex sheet they can cause unexpected behaviours.
https://developer.mozilla.org/en-US/docs/Web/CSS/CSS_box_model/Mastering_margin_collapsing
Well it's not that unexpected. They collapse! 😉
|
|
||
| .header h1 { | ||
| flex-grow: 0.75; | ||
| font-size: 2rem; |
There was a problem hiding this comment.
The font scale seems a bit random.
Typically in a design we define a font scale - perhaps put this at the top of the page with your colour palette and your spacing - and then pick sizes from that scale.
| @media screen and (min-width: 665px) and (max-width: 1120px) { | ||
| .App { | ||
| padding: 0 20px; | ||
| @media screen and (min-width: 765px) and (max-width: 1120px) { |
There was a problem hiding this comment.
This is a curiously specific sizing rule. Could this be a magic number?
https://css-tricks.com/magic-numbers-in-css/
What problem are you trying to resolve with this - is it possible you could resolve with one of these layout classics:
| .App .cyf .about-trainees figure { | ||
| .cyf .about-trainees figure { | ||
| grid-template-columns: repeat(4, 1fr); | ||
| gap: 0px; |
There was a problem hiding this comment.
| gap: 0px; |
If you removed this, what would happen?
2 participants
CYF fontandCYF faviconapplied.pure HTML.READMEis in progress...Performance: 96
Accessibility: 100
Best Practices: 95
SEO: 90