First pull request for solo project #27
Conversation
There was a problem hiding this comment.
Generally:
- Using rem instead of px for most things (except stuff like border-radius, border size, box-shadow etc) can make responsiveness easier (imo) as you can just change the overall font-size of the html element (in %) and it adapts accordingly to allow for (usually) fewer queries changes :D
- Sometimes when css gets too long, making a new queries.css can help, but its case-dependent.
- You mentioned this in your demo but thought to write a comment as, but the green on yellow may be difficult to read especially accessibility-wise (for the mobile nav).
There was a problem hiding this comment.
(Deleted the previous review as I didn't realise it had saved it after I exited the browser)
There was a problem hiding this comment.
You've done this in your CSS so I'm sure you're aware of it as a tactic but thought to throw a comment in here anyway. You can also use comments in JS to separate your JS code (when it gets longer of course).
There was a problem hiding this comment.
I only really had small tips and things that help me with code organisation and useability stuff as overall it looks well-structured and I've seen the demo of the website and it looks good (and works well) so no major feedback and a great job on all fronts, well done :D
There was a problem hiding this comment.
Good job with this project Bianka! You’ve managed to create a responsive website using flexbox and CSS Grid and you made it interactive. I would maybe let the hamburger stay for a little bit larger screens as well, even though the links fit. This works fine however 😇
Don’t forget labels for each form input! It’s important for accessibility. This needs to be fixed in order to be approved. Apart from that - good job!
PS. We will talk more about accessibility later, but another thing you can think about already now is contrasting colors. Yellow on white is not so easy to read 😅
Changes requested
- Labels for all form elements
|
Ping! ☝️ |
Thank you for reminding me!! I am so sorry I got caught up in the JS projects, I did not realize I left this unattended. I think I have followed up as needed as noted in my recent commits. And in my review of "label" use, as I understand it, when we nest "input" inside of "label" that associates and labels the input correctly. I added explicit labels to the name and email inputs that were missing it, but wonder if I understand it correctly for the checkbox inputs? Please advise if I am missing something 🙌 Thank you again!! p.s. sorry if it spammed you of my edits. I had written the quoted words using the arrows as in HTML and it was disappearing them when I submitted the comment, so had to edit to make sure it was written out correctly. 🫠 |
Hello, thanks for your honest feedback on my first site.
I have not yet updated my Javascript additions because its been ruining my navbar. I will hopefully have it done by tomorrow. But otherwise I think I am content with a first shot at this. I intend to actually write-up real blog posts, but the code tweaking has sadly taken up all my time, so stay tuned for updates if you want some nice Stockholm recommendations☺️ -B
p.s. after finally getting the ham menu up... I was testing the responsiveness. So just note: if in Inspect mode, mobile templates trigger the ham menu, but just rescaling the web browser directly does not. Not sure if that's a bug on me, or the browser knowing its not a mobile view no matter if its as small. Happy to get feedback on that too.