Mark Teekman
@markteekmanAll comments
- @Haico-PaulussenSubmitted over 2 years ago@markteekmanPosted over 2 years ago
Hey Haico! Love your use of Marvel quotes, big fan of Marvel myself 🙂 Don't know if it's possible with the API you've used, but it would be cool to see the person behind the quote e.g. "You took everything from me." - Wanda. Just an idea though.
To improve the accessibility and the usability of your solution a bit, consider using
cursor: pointer;
for when you're hovering over the randomization button.Great work Haico!
Marked as helpful0 - @legion40216Submitted about 3 years ago
Any improvement Feedback
@markteekmanPosted over 2 years agoLove how you've added that "Best of three" at the top of your solution Suleman! Really creative 😊 One tip would be to checkout the report Frontend Mentor generates for your solution, there's some really great tips on how to improve your HTML and accessibility.
Keep up the great work!
1 - @NohaFahmiSubmitted over 2 years ago@markteekmanPosted over 2 years ago
I really like your solution as well Noha! Especially the way you animated how the house picks its icon 🙂 Keep up the great work!
3 - @hiddehulshofSubmitted about 3 years ago@markteekmanPosted about 3 years ago
Hey Hidde,
Nice solution! It's really close to the original design :) Be sure to also check Frontend Mentors report, it contains really helpful information about accessibility and semantic HTML! In this case, adding an empty
alt=""
tag to your image and wrapping your content in a<main>
landmark should do the trick.Keep it up!
Marked as helpful0 - @Duyen-codesSubmitted about 3 years ago
I hardcoded the data since I got stuck at injecting data from JSON file. Would really appreciate if you can give some tips on this problem. Thanks a lot.
@markteekmanPosted about 3 years agoHi Duyen,
Great solution! Love how you combined Grid en Flex to build this one out :) Be sure to also check Frontend Mentors report, it has some great tips to improve accessibility! For example, adding an empty
alt=""
tag to all your images should solve all the issues in this case.You could also improve the design a little by making 'daily', 'weekly' and 'monthly' into anchor links with an active and hover state :)
As for the data, I'll reference it here again so others might read it too: the JavaScript Fetch API can be very useful in this case. You'll need some JS to interact with the data, there are plenty of ways to do it, this is how I solved it.
Keep up the great work!
Marked as helpful0 - @nicktelindertSubmitted about 3 years ago@markteekmanPosted about 3 years ago
Hi Nick,
Great first solution! Haico has already made some nice additions. Things I could think of (I took some of these from my comment at Haico's solution):
- When viewing the component on a small screen size the card overflows the viewport. This is because you've set a fixed
width: 302px
on the#card
element. Try to avoid setting it explicitly as much as possible. Settingmax-width: 302px
solves this. - Consider using classes instead of id's to style your HTML. It's lower in CSS specificity and a better practice for re-usability.
- Your anchor links aren't distinguished in this component. 'Equilibrium #3429' and 'Jules Wyvern' are links, but this is not visually notable. Consider adding an underline to them for accessibility purposes.
- Your attribution links have a color contrast ratio of 1.88. The minimum contrast ratio is 4.51, a nice one would be a ratio of 6.0. You could achieve this by using a white color and an underline for example.
- You can add
cursor: pointer
to your.overlay:hover
class to add that extra level of interaction taken from the design :)
Keep up the great work and happy coding!
0 - When viewing the component on a small screen size the card overflows the viewport. This is because you've set a fixed
- @Haico-PaulussenSubmitted about 3 years ago@markteekmanPosted about 3 years ago
Haico, just figured I forgot one addition, you can add
cursor: pointer
to your.header:hover .view-container
class to add that extra level of interaction taken from the design :)1 - @Haico-PaulussenSubmitted about 3 years ago@markteekmanPosted about 3 years ago
Hey Haico,
Great work once more on this challenge! You can definitely see your CSS Grid skills improve :) I like how you kept the HTML very minimal and used the power of Grid to handle the structure of the layout.
Some small improvements:
- On smaller screens (phone for example) the attribution footer lays over your card because you've used
position: fixed
. You could try something like a sticky footer solution, where it's still at the bottom on large screen sizes but stays below the main content on small screen sizes. - When viewing the component on a small screen size the card overflows the viewport. This is because you've set a fixed
width: 375px
on themain
element. Try to avoid setting it explicitly as much as possible. Settingmax-width: 375px
solves this. - Try to include a CSS reset in your project, not including one can often lead to differences between browsers. A good example is trying to implement the sticky footer I mentioned. You can see in Firefox that the body has some margin which results in the footer overflowing the viewport because of the
width: 100vw
the footer has. There are many options, a simple one is all you need. - When using
alt=""
attributes on images, check whether is makes sense to put it there. For example, you havealt="image of Jules Wyvern"
on the avatar, followed by the text 'Creation of Jules Wyvern'. A screen reader would now read 'image of Jules Wyvern Creation of Jules Wyvern'. So here, it's better to have an emptyalt=""
tag. If the avatar image wasn't accompanied by any text it would make sense to set the alt tag to describe it to the user. - Another accessibility concern is that your anchor links aren't distinguished in this component. 'Equilibrium #3429' and 'Jules Wyvern' are links, but this is not visually notable. Consider adding an underline to them.
All in all, it's a great looking component!
Happy coding :)
Marked as helpful2 - On smaller screens (phone for example) the attribution footer lays over your card because you've used
- @Haico-PaulussenSubmitted about 3 years ago
I'm very proud of my work. But I want to learn how I can do things better. Especially using CSS Grid. So if you have any comments or improvements I really want to know so I can keep on learning and getting better!
@markteekmanPosted about 3 years agoHey Haico,
Great solution using Grid! I like how you’ve implemented a couple of the shorthand’s and things like min-content, nice use case!
Also, don’t forget to check Frontend Mentor’s Accessibility Report, it’s a good way to learn about some fundamental HTML stuff.
Just so it happens, Kevin Powel (the CSS Wizard) did a YouTube video on CSS Grid using this exact same Frontend Mentor challenge as an example, would be a nice way to compare different approaches: https://www.youtube.com/watch?v=rg7Fvvl3taU.
Keep up the great work!
Cheers, Mark
Marked as helpful1 - @Artmade-studioSubmitted about 3 years ago@markteekmanPosted about 3 years ago
Very nice solution you made here! I really love seeing you've used Astro :) I've got a couple of pointers to further improve your code and the user experience of your solution:
- Your menu items are all
<h1>'s
, for accessibility it's important to only use one<h1>
per page. You could instead just put your menu items in the<a>
tag alone and apply the necessary styling there - When hovering over the menu items they become bold and this creates a shift in layout. You can solve this by setting your menu items inside unordered list items and apply a
min-width
on them to prevent this shifting. I did the same for the menu in this solution: https://markteekman.github.io/easybank-landing-page/ - Consider adding
cursor: pointer
to your light and dark mode toggles to make it more clear to the user that it's interactive - On a single product page you can apply a
gap: 2rem
property on the parent div that wraps the little preview images to give them some space like the original design - Consider putting some padding on the container for smaller viewports on the single product page, that way there will be some breathing room between the big image and the browsers edge
- Checkout Frontend Mentors report on your solution, it's a great way to learn more about accessibility and correct HTML mark-up
Hope these help, keep up the great work and keep it up with Astro! :D
Happy coding! Mark
Marked as helpful1 - Your menu items are all
- @Haico-PaulussenSubmitted about 3 years ago@markteekmanPosted about 3 years ago
Hey Haico, nice first solution, looks great 👍🏼
Some suggestions on how to improve your code:
- You could try using a
<footer>
landmark instead of a div with a class of footer for better markup - Also wrap your card in a
<main>
tag for a better and more accessible markup (these first two points should tackle most of your accessibility warnings) - You could set the background on the
<body>
instead of using a separate div to clean up the code a bit
Keep up the good work!
Marked as helpful1 - You could try using a
- @MikevPeerenSubmitted about 3 years ago
Hello all,
I would mainly want to know if the styling could be improved. Any other feedback is also welcome of course :D
All issue's displayed on Frontend Mentor are related to Next.js, if there are also tips on how to remove them please advise.
@markteekmanPosted about 3 years agoNice one Mike!
Looks really great and some great suggestions by Dharmik48 already :) Some additions:
- You can also improve the design a little more by setting the ProfileCard width to 20% to make it resemble the original design a little more.
- There's an error in the console about an uncaught promise, but maybe that's also a Next.js thing because switching the stats does work.
Keep up the good work!
Marked as helpful2