@vanzasetia
Posted
š Hi Giovanni!
š Congratulations on finishing your second challenge! Hopefully, this challenge increase your flexbox skill š. I really like the eye animation when I hover the equilibrium, so good job on this one!
I directly notice one odd thing when I see your repo. Your branch name is mainbranch. You don't need to put the word branch, just put the name of your branch, in this case, main.
More feedback:
- Accessibility
- Create a custom
:focus-visiblestyling to any interactive elements (button, links,input,textarea). This will make the users can navigate this website using keyboard (Tab) easily. - In this case, any elements that have an active or hover state should be interactive elements (link or button). Try to read about when to use a link or button.
- For any decorative images, each
imgtag should have emptyalt=""andaria-hidden="true"attributes to make all web assistive technologies such as screen reader ignore those images. In this case, all images are decorative only except the equilibrium and the avatar. - For the equilibrium (or dice) image, the alternative text should tell the user what will happen when the user clicks that image. For a better understanding of the alternative text, I would recommend reading the Alt texts: The Ultimate Guide article by Axess Lab and trying to follow this decision tree to decide how to use the
altattribute in various ways. - For the avatar image, use the name as the alternative text of his photo.
alt="Jules Wyvern" - Every page should have one
h1. In this case, you can make theEquilibrium #3429text as a heading. - All page content should live inside a landmark (
header,main,footer). In this case, you can wrap all page content withmaintag except your attribution. For the attribution, swap thedivwithfooter. - I would recommend using
remunit as the unit of the bodyfont-size. Usingpxunit won't allow the user to zoom or scale the page. It's also important to makebodyfont size using relative unit (rem) since most HTML elements inherit the font size of thebodyelement.
- Create a custom
- Styling
- On landscape mobile view (640px * 360px), the card having full height. You can prevent this by adding
paddingto thebodyelement. - Always make all elements
box-sizing: border-boxas your common reset. This will make sure that all elements behave the way they should. To learn more, you can search about it.
- On landscape mobile view (640px * 360px), the card having full height. You can prevent this by adding
html {
box-sizing: border-box;
}
*, *:before, *:after {
box-sizing: inherit;
}
- There's no need to set
height: 100%on thehtmlelement. It's not useful, as far as I know, since the main container or element is thebodyelement, nothtml.
That's it! Hopefully, this is helpful!
Marked as helpful
@Gioltek
Posted
WOW! @vanzasetia this is a really invaluable feedback! Thank you so much for taking the time to read my code and write all of these suggestions, you are so kind!
I'm going to answer all of your points:
-Accessibility
-- :focus-visible: yes, that's surely something I should start doing, thanks for letting me know! I fixed it on the live website, is it okay now?
--Fixed the alts as you suggested, they should be fine now! By the way, only the second link you sent me works, the frontend one doesn't for some reason :(
--The "Equilibrium" title is now a h1!
--Added the main section and the footer section!
--Fixed the body font-size! Now it is 1.125rem (which should be 18px according to my google search, which makes sense as 1rem should be 16px).
-Styling
--Fixed padding on body so that the card isn't 100% height on mobile!
--Put your string of file for the box-sizing, next thing I'm gonna do is read about it! I saw someone using it on their code but I never understood it, so I'm definitely gonna look it up!
--For some reason my html border (I also highlighted it with a border on css to be sure) is way too small, it ends just below the .attribution. If I don't make it 100% height my cardbox won't be centered in the middle of the page, it will just be centered on top of the page for some reason. Do you have any tips about that?
-Other
--Thanks for the tip about branches, I definitely should learn github. I just saw a tutorial on how to upload code on it so that I could submit my solution, and the tutorial guy named it "mainbranch" so I just copied it ahah!
--I'm glad you enjoyed the eye animation! I would love to tweak something here and there in challenges like these, but the main point of them is to create something as much similar as the original layout so I can't do much, but these eye thing was a funny way to experiment. Thanks again for all your tips and sorry if I got something wrong from your points or if I misunderstood something, have a nice day!
@vanzasetia
Posted
@Gioltek Glad to know that it is helpful for you! š
- Accessibility
- The current
:focus-visibleis not as clear (almost the same as the default). I would recommend trying this styling instead and seeing the difference.
- The current
:focus-visible {
outline: 0.1875rem dotted #00fff7;
outline-offset: 0.3125rem;
}
- š Sorry for the second link, here is the correct link.
- š Good job on fixing all those issues!
- Styling
- About
height, I would recommend settingmin-height: 100vhonbodyelement and removing theheightproperty onhtmlelement. Search aboutvhunit, if don't familiar with it.
- About
- Additional Feedback
- I notice one issue, where you can't make
h1as the child ofa. It's potentially invalid. Swap it and put theatag inside theh1.
- I notice one issue, where you can't make
Hopefully, this answer all your questions!
P.S. Generate a new report and see is there any issue left or not.
@Gioltek
Posted
Thanks again for the reply @vanzasetia!
I tried your :focus visible and yes, it is definitely more clear than before. I was trying to do something easier on the eye, a bit more aesthetic. Would you say that in the case of :focus I should choose legibility over something "prettier" but more difficult to spot?
100vh worked like a charm, so thanks for the tip! I knew about viewport units but I used them interchangeably with %, I should read more about them so that I know when to use one and when the other. And also thanks for the anchor tag and h1 "incompatibility", I fixed it!
I also generated a new report just like you suggested and it seems flawless structurely speaking, so thanks for your tips! I hope to find more of your comments in my next challenges, you're really good at this! Have a nice day/good night! :)
@vanzasetia
Posted
@Gioltek Yes, I would focus on the legibility of the :focus and :focus-visible style, since the purpose is to make sure that the user can navigate the website using keyboard easily.
Marked as helpful
