Natasha Pierre-Louis
@natashaplAll comments
- @InitiunsSubmitted about 2 years ago@natashaplPosted about 2 years ago
Hi Filipe. Nice work! I only noticed a couple of things. There are three notifications at the top, but after I mark all of them as read, there's still one at the bottom of the page that is marked as unread. As for accessibility, I suggest you add an h1 to your header and rerun the reports in the "View Reports" page to confirm all accessibility issues are resolved.
Hope this help. :)
0 - @Glenda9031Submitted about 2 years ago
I'm confused about the order of the selectors in CSS (should display go before margin, width, height, etc.). I also am REALLY confused about media queries even though I have at least 3 certifications (Codecademy, Scrimba and Free Code Camp) but I still be confused!!
@natashaplPosted about 2 years agoHi Glenda. Nice work! I think most issues were already addressed. The only thing I wanted to point out is that the commented message you're using to separate the
header
from themain
tag is causing you page to not be valid HTML. The fix is to remove some of the hyphens since there should only be 2 on each side: Once you're done fixing it, you can regenerate a new report by clicking the "View Report" button above and and then on that page click "Generate Report".I hope this is helpful. :)
Marked as helpful1 - @MorbitDemonSubmitted about 2 years ago
-
This was my first time doing Mobile design first.
-
The most annoying part of this challange was an error i had with git/github that didnt let me upload the respository.
-
Any feedback is appreciated
@natashaplPosted about 2 years agoHi Deren! You did a great job for a first responsive solution. The only feedback I have is that there seems to be an excessive amount of margin around the
section
andattribution
. I can tell you were trying to match the design, but it may be best to vertically center the section. Since you're using CSS grid, you can apply the following CSS rule to themain
tag:.main { display: grid; height: 100vh; align-items: center; }
As for the attribution, consider decreasing the margin some, unless your intention is to hide it below the viewport.
I hope this is helpful. :)
Marked as helpful1 -
- @arturo0427Submitted about 2 years ago
recommendations?
@natashaplPosted about 2 years agoHi Arturo. Nice work! Just a couple of things I noticed when I checked out your code. For accessibility reasons, headers such as
<h1>
should increase by one. What this means is that since you used a<h1>
in your header, the next header text should be<h2>
rather than<h4>
.Also, in order for your HTML to be valid, you'll need to add headers to all of the bottom sections. I suggest replacing the
<span>
tag around the names with<h3>
tags.I hope this helps. :)
1 - @willie10rSubmitted about 2 years ago
Would like to mainly know what your thoughts are on my js. "Feedback welcome".
@natashaplPosted about 2 years agoHi Willie. Congrats on uploading your first solution and welcome to Frontend Mentor! Just a couple of things I noticed. I haven't done this challenge yet, but I was looking at the design images and think on hover the background should be gray with white text. On click of a rating the clicked button should be orange.
Also, it appears in the `<svg class="svg-1"> you spelled "height" incorrect and that's causing your HTML to be invalid.
Last but not least, to remove the accessibility issues in your report, consider wrapping both the
<div class="before">
and<div class="after">
in the<main>
tag. It helps with screen readers.I hope this is helpful. :)
1 - @KTrick01Submitted about 2 years ago
Hi! I wanted to feel a spike in difficulty, so I left my junior challenges for a while and went for an advanced challenge, and I have to say that it was pretty fun to make, I definitely want to do more projects like this (though there's just one more free advanced challenge i think 😥)! I learned a LOT of Vue doing this challenge, actually I think I spent more time watching tutorials and reading documentation than anything else, my solution it's far from perfect so I will be trying to optimize the code. As always any feedback is welcome!
@natashaplPosted about 2 years agoHi Eduardo! Your game is super cool! Nice work! I only noticed a couple of things:
- After I won a few rounds, the score remained "1".
- There appears to be the following console error, so perhaps its related...
VM87:85 TypeError: Cannot read properties of null (reading 'close')
I hope this is helpful
1 - @MaikoCodeSubmitted about 2 years ago
Let me know your comments and advice !!?
@natashaplPosted about 2 years agoHi Mamadou. Nice work on the slider! If you'd like to improve accessibility and remove that issue from your report, consider changing the
<div class="container">
to<main class="container">
. Using semantic tags likemain
helps improve accessibility. Here's an article that discusses the benefits of semantic tags:HTML5 Semantic Tags: What They Are and How to Use Them!
I hope this is helpful. :)
Marked as helpful0 - @DaveAigbeSubmitted about 2 years ago@natashaplPosted about 2 years ago
Hi Dave! Congrats on uploading your first solution and welcome to Frontend Mentor! Also, nice work on your solution! If you'd like to get rid of the accessibility issues that's in your report, consider changing the
<div class="container">
to<main class="container">
. I also suggest changing<div class="attribution">
to `<footer class="attribution">. These HTML 5 tags help improve accessibility for screen readers. Here's an article that discusses the benefits of semantic tags:HTML5 Semantic Tags: What They Are and How to Use Them!
I hope this is helpful. :)
Marked as helpful1 - @casey-botSubmitted about 2 years ago
Hello! I'd love to hear any feedback about the organization/legibility(?) of my code. I'm really new and am not sure what to compare it to for the purpose of self-evaluation. Also, any feedback on how I can get my solution to better match the design is very welcome! For my first few solutions the scale of the card was a bit off. Thanks!
@natashaplPosted about 2 years agoHi Casey. Your solution looks nice! I have a couple of suggestions per your question:
-
I recommend that when you upload a solution, you click the "View Reports" button if it shows any Accessibility or HTML issues. This report gives valuable insight on things you can fix or improve.
-
For the
article class="image"
, I recommend you add the source image as a background image using CSS rule in your index.css stylesheet rather than adding the source in the markup. That source tag causes your HTML to be invalid.
Here's an example of how that would look:
.image { position: relative; background: url(./images/image-header-mobile.jpg) no-repeat 0 0; height: 100%; }
-
It's not valid HTML to place a
p
tag inside anh2
. Use aspan
instead and if you want you want the element in its own line applydisplay: block;
to the span. -
Consider changing the width of
.container
towidth: 1080px;
for desktop and then adding anoverflow: hidden;
to keep the content from flowing outside the rounded corners. I also suggest you change the height of.container
toheight: 68vh;
.
I hope this is helpful. :)
Marked as helpful1 -
- @69kwan69Submitted about 2 years ago
My first solution to the challenge that includes Javascript, and the JS part seems to behave weirdly. Open the DevTools > Console and click any rating number to see what I mean. If you know how to solve it, it would be much appreciated.
All feedbacks are wellcome, especially the JS one.
@natashaplPosted about 2 years agoHi Trvan. Nice work! It looks like it's throwing an undefined because it's targeting both the input and the span inside of
.rating-btn
div. Adding theinput
to theconst ratingBtns
worked for me:const ratingBtns = document.querySelectorAll('.rating-btn input');
I hope this helps.
Marked as helpful1 - @trandainienSubmitted about 2 years ago
All comments are welcome
@natashaplPosted about 2 years agoHi Nien! Your solution looks really good. The only feedback I have is that the cyan on the bottom right seems a little too bright and makes the text a little hard to read. Perhaps try a lighter value of the original cyan provided in the style guide. Perhaps something like this:
.bg-lightCyan { --tw-bg-opacity: 1; background-color: hsl(179, 62%, 47%); }
I sorta eye-balled it. You can tweak the "47%" up or down until it closely matches the color in the design image. I hope this helps.
Marked as helpful1 - @jaggycodesSubmitted about 2 years ago
All feedback is welcome!
@natashaplPosted about 2 years agoHi Sadas! Welcome to Frontend Mentor. I'm new to this platform as well and recently uploaded the same solution. I took a look at your solutions and it's pretty good. I have a few suggestions.
- Consider wrapping your content in the
main
tag. It helps screen readers and is semantic HTML. You can learn more about the benefits of semantic tags here:
HTML5 Semantic Tags: What They Are and How to Use Them!
- Once you place your content in that
main
tag, you can use it to center you content vertically and horizontally. You can remove theposition: relative
and "top" and "left" from the "background" since you won't need them for positioning . Here's a CSS rule you can try for the main tag:
main { display: flex; align-items: center; justify-content: center; min-height: 100vh; }
- once all your content is centered with the main tag, you can also remove all relative position styles from the content. For example, the
qrcodeimage
can become:
.qrcodeimage { max-width: 100%; width: auto; border-radius: 10px; }
The max-width: 100% is to ensure that the image is fluid.
- Consider hanging the
headerone
div to an h1 tag. It is best practice to always have an h1 tag on your web page.
I hope this helps.
2 - Consider wrapping your content in the