Design comparison
Solution retrospective
Feedback is much appreciated. Thank you!
Community feedback
- @pikapikamartPosted almost 3 years ago
Hey, great work on this one. The layout in general looks fine to me, but when opening up dev tools at the bottom, the wavy-background-image suddenly changes size. The site is almost responsive because at some point like 380px width, the layout is being hidden by the screen, creating a horizontal scroll. On layout like 350px, the text is almost squished and the bottom section on the pricing, the music-icon is overlapping its right-side element.
Diego Serrano already gave feedback on this one, just going to add some suggestions as well:
- It would be great to have a base styling of this:
html { box-sizing: border-boxl font-size: 100%; } *, *::before, *::after { box-sizing: inherit }
This way, handling an element specially its size will be easier because of the
box-sizing
- Instead of using
background-size: contain
usebackground-size: 100%
so that it will fill up all the spaces properly. body
tag does not needfont-size: 16px
since by default it uses that value.- Avoid using
height: 100vh
on a large container like the.box
as this makes the element's height capped based on the viewport/screen's height. Instead usemin-height: 100vh
so that the element will expand if it needs to. - To make the layout more adaptive, in general, instead of using
width
usemax-width
to limit a size of an element. This way, even if the size gets lower than the size on themax-width
the layout will respond on it since it doesn't have a fixedwidth
. For example, usemax-width: 450px
on the.box-grid
and usewidth: 100%
on theimg
inside it. You will see the different if you change the screen-size via dev tools. - The vector/illustration is only decorative on this one. Decorative images should be hidden for screen-reader at all times by using
alt=""
andaria-hidden="true"
to theimg
tag or onlyaria-hidden="true"
if the image is usingsvg
. - Only use descriptive
alt
on images that are meaningful and adds content to the site otherwise hide the image for screen-reader users. - The music-icon is only decorative as well so it will be better if it was hidden as well.
- Also, when using
img
tag, you don't need to add words that relates to "graphic" such as "icon" and others, sinceimg
is already an image so no need to describe it as one. - The
annual-plan
text could have used a heading tag likeh2
since it gives information on what the section would contain, hence the pricing of the plan. - You can just use
a
tag directly or usediv
to contain and instead of usingp
tag. - You don't need to use
role="navigation"
on the 2-buttons since those aren't really navigational links. Since you usedbutton
on this one, I assumed you though this component as aform
wherebutton
is used. So maybe it will be better if you usebutton
as well on thechange
but, only if you will wrap the layout inside of aform
. - Lastly, just addressing the layout issue on like 350px size below:>
Aside from those, great job again on this one.
Marked as helpful0@mikkoolPosted almost 3 years ago@pikamart First of all, I am so thankful for this comment. Just so you know that the first time I joined this community you are my inspiration. Learning html, css and javascript for almost two months now. I am really having fun and I don't know how I end up to your feed but I have seen your works and I said wow. You're amazing and finding out that you too are filipino. Makes a win win for me :) Thank you for this helpful comment. I always wanted to ask you to be my mentor but I am scared to ask. Lol. But this, this comment! Super! Super made me happy.
Thank you so much! Really, really appreciate it :)
1@pikapikamartPosted almost 3 years ago@mikkool Glad that you find the feedback useful and there are only few Filipinos that I encountered as well ( but we'll just stick to using english so no excluding:> ) including you.
Thank you as well for my works, but do not take notes of other solutions that I made since I haven't refactored those old ones, I've been refactoring others but can't find a dedicated time for it.
Also, I'll give you a follow now so that I could see your other submission as well and I could review it as well. Goodluck for every challenges^^
Marked as helpful0@mikkoolPosted almost 3 years ago@pikamart Thank you so much :) I really appreciate it. I am really new at this :) Not sure if I am doing the right thing. Lol. Thank you!
1 - @dserranoiPosted almost 3 years ago
Hey @mikkool - cool page. 1 comment if you don't mind. 1.- Your repository is not found. .
Marked as helpful0@mikkoolPosted almost 3 years ago@dserranoi Thank you for checking out, I don't know why it says repository not found. I linked the right one. Just in case here: https://github.com/mikkool/order-summary-component-main
Thank you so much!
0@dserranoiPosted almost 3 years ago@mikkool Still not able to see it, did you set it up as private?
Marked as helpful0@mikkoolPosted almost 3 years ago@dserranoi Oh. My bad, yes it was in private. It's public now :) Thank you!
0@dserranoiPosted almost 3 years ago@mikkool awesome i love your code, nice!
Marked as helpful0@mikkoolPosted almost 3 years ago@dserranoi Thank you, hope that helps, I am learning as well. Good luck to our journey :)
1
Please log in to post a comment
Log in with GitHubJoin our Discord community
Join thousands of Frontend Mentor community members taking the challenges, sharing resources, helping each other, and chatting about all things front-end!
Join our Discord