somaye beiranvand• 220
@somaye-beiranvand
Submitted
Hello every one😊
Welcome to my new solution🌷🌷🌷
Any feedback on code or design is appreciated 🙏🙏🙏
P
James
@james-work-account
All comments
A couple of immediate things:
- In Firefox, your radio buttons are still visible (albeit tiny). You can set something like
-moz-appearance:none;but this will make keyboard navigation look a bit odd. Worth investigating. - There isn't any feedback that the selected radio button worked - it would be good to keep the active radio button visibly selected.
- Your "Rate us again" element needs to be a
<button>not a<div>. You should never have interactive elements which aren't buttons, inputs or links.
Marked as helpful
0- In Firefox, your radio buttons are still visible (albeit tiny). You can set something like
- Rohan Agroya• 150
@rohanagroya
Submitted
This looks great! You have a few accessibility/usability issues though:
- You need to use buttons or links for interactive elements.
<img>tags shouldn't ever have onclick listeners. The most basic way to test if your site is usable by people with accessibility needs is to see if you can tab through the interactive elements or not (ie try to use your website without your mouse/trackpad) - on your solution, you currently can't - Missing a
<main>tag - Your images don't have accessible names (aria-label or sr-only text)
- Your viewport <meta> tag is wrong, so accessing on a phone looks really small/awkward. You should change
<meta="viewport" content="width=device-width, initial-scale=1.0" />to<meta name="viewport" content="width=device-width, initial-scale=1.0" /> - When adding in a proper meta tag your styling will need changing as it looks a bit messed up for me when I try to use it from a mobile device with the correct viewport tag
- You're missing the "Rules" button/popup
Marked as helpful
0 - You need to use buttons or links for interactive elements.
- Sachintha Prasad• 310
@Sachintha-Prasad
Submitted
Hello Friends!! This is my new challenge😘Highly appreciate your feedback 🤩.
Happy coding 😊❤️
Looks great! Just a handful of of things I noticed:
- Your website overflows between ~769px and ~843px, you should adjust your media queries to ensure there isn't any unintentional overflow.
- All elements with hover effects should be either anchor tags or buttons, even if the anchor tags have a href of
#or the buttons don't have an associated function. So in your case, your Facebook, Twitter and Instagram icons should be wrapped in anchor tags (you can probably just replace<div class="social-icon">...</div>with<a href="#" class="social-icon">...</a>). - Your hover effects should also apply to the
focus-visiblepseudo-class. This means that people who mostly navigate websites using their keyboard rather than mouse (such as myself) can have a visual indication that they've tabbed to the correct element. So for example,.register-btn:hovershould be.register-btn:hover, .register-btn:focus-visible. - This is a problem that a lot of people who are relatively new to SCSS face - avoid heavily-nested SCSS. A good rule of thumb is that you shouldn't really need to nest SCSS selectors other than pseudo-classes or generic selectors like
.my-class + *. There's really no need to write something like.hero-container .hero-content .register-btn, since your.register-btnisn't being used anywhere else. If you take a look at your outputted CSS, it should end up looking like it was written without SCSS, not like heavily nested spaghetti.
Otherwise, this looks really clean. Good job!
Marked as helpful
1 - Felix Pherry• 40
@felix123136
Submitted
What's the best practices for this challenge?
A great start! Some things which would be good to change:
- Remove the margin on your
<body>element to avoid unnecessary vertical scroll. Related: it would be worth looking up a good CSS reset as general "good thing to do when starting a new project", e.g. https://piccalil.li/blog/a-modern-css-reset/ . - Change
height: 100vhtomin-height: 100vh. You don't want to set explicit heights on things since height of an element should be decided by its contents. However if you want something to be "at least X height", usemin-height. In this challenge it doesn't matter too much since the component is small, but it's a good habit to get into early. - It's great that you've used a
<main>element! Something to consider would be to change<div class="attribution"> to<footer class="attribution">`, since a footer is a landmark and your footer is outside your main element. It's all about using the correct semantic element, and this will fix your remaining Accessibility Issue. - Consider moving away from px for widths/heights/font-sizes/etc and switching to using rem for this. There are a few articles explaining why you'd want to do this (e.g. this one) - there are online px-to-rem converters if you want to make your life easier.
- Style on classes not ids (e.g. add a
.cardclass to your#cardelement and style with the class not the id). A general rule of thumb is that classes should be reserved for styling and ids should be used for things like linking up inputs with labels, making something selectable with javascript, making hyperlinks on a page, etc.
Some stylistic things to consider:
- Instead of having a percentage width on your image, leave it at 100% and rely on padding (using rem) to shrink it. Not a major thing, just something to think about (leaving widths alone and using padding/margins/flex/whatever to influence size).
- I guess you could link to your GitHub or Frontend Mentor profile in your attribution (it currently just links to
#).
Overall though, this is a really solid submission. Keep it up!
0 - Remove the margin on your
- arey• 350
@arey-dev
Submitted
I'm glad I was able to finish this design, but I wasn't satisfied of my solution. Also, one thing I've been confused at is the scrollbar. What I did on this project was add
paddingon thebodyelement to have scrollbar effect, but I'm not sure if that was the best practice. Can you give me some tips or advice? Thanks!Looks great!
Something to consider, why did you put min-widths on your
.featureand.containerelements? Any people with phones below ~400px wide (e.g. iPhone 13 mini) will experience horizontal overflow, which is something you'd generally want to avoid.Also, consider moving away from px for widths/heights/font-sizes/etc and switching to using rem for this. There are a few articles explaining why you'd want to do this (e.g. this one) - there are online px-to-rem converters if you want to make your life easier.
Marked as helpful
1 - Yahya Ahmed Farouk• 530
@yahyaahmed1
Submitted
- easy and amusing.
- responsive design.
Your solution needs some content landmarks - the
.boxdiv should be inside amainelement and your.attributiondiv should be a footer. This is for accessibility reasons, as it tells assistive technologies (e.g. screen readers) where everything is and makes your site easier to navigate for someone with disabilities (plus it will remove a lot of your Report issues).This project doesn't really require relative positioning - have a look into positioning elements with flexbox. For example, on my screen everything is off-centre since everything's positioned with px values (have a look at your solution from sizes in-between and either side of the given screen sizes in the design files). You should be able to use flexbox and not need any media queries at all, since the element won't stray from the centre of the screen.
You may want a max-width on your
.boxbut shouldn't be setting hard widths on elements - for this challenge it doesn't matter as much, but if someone has a very small screen, or they are viewing the website split-screen (with something else next to it), etc, your elements will overflow off the edge of the screen. Same goes with height - you may need a max-height, but generally you'd have the height dictated by the container's contents.This is a good start. I suggest you challenge yourself to rewrite your CSS removing all media queries and fixed heights/widths, whilst keeping the solution looking the same. Good luck!
0 - AlterNateUwU• 20
@AlterNateUwU
Submitted
Your live URL and code URL aren't working - please make sure you've put them in right
0 - Wassem Samir• 60
@wassemsamir
Submitted
This is a great start! Just some notes to improve:
- Missing box-shadow on the container
- You have skipped heading levels - there shouldn't be a h3 without a h2, there shouldn't be a h2 without a h1, etc. The h3 should be a h1 and re-styled if it is too big
<img>could do with an alt property - it is not obviously described by any other text on the page and isn't purely there for aesthetics- It would be good if the card was vertically centred
- Your container could be a main, your attribution could be a footer. This would remove some of the accessibility issues that the automated tool has raised
- Some of the padding/font-size values could be tweaked to get it closer to the original design (though honestly this isn't a priority)
Otherwise this is a really good submission. Keep it up!
0 - CMarghin• 40
@CH4R4F
Submitted
Looks great! Just a pointer to make sure the items are all the same height on Desktop - try taking
align-items: centeroff the.containerclass.Marked as helpful
1 - Fraser Watt• 1,790
@fraserwat
Submitted
This was a fun one!! Trickiest part was working with images sizes, definitely the thing in front end I find hardest.
Still not 100% pleased with the "Graphic Design" / "Photography" section and how that changes at different page widths, so any pointers on how to approach this so the image responds more smoothly (and I don't have to rely on background colours at some points) v much appreciated! 😎
A couple of small things I noticed:
- On mobile view, the Graphic design and Photography
<p>tag text is a light grey rather than dark green/blue respectively - On mobile, if you open the hamburger and switch to desktop view, the dropdown menu stays open instead of going back to the top
Otherwise it looks great! I've been struggling to change the colour of the sunnyside logo in the footer, I just resorted to changing the colour in the svg file itself. Interesting solution to use a filter on it, I hadn't thought of that.
1 - On mobile view, the Graphic design and Photography
