London class 10 - Iryna Lypnyk - HTML-CSS-Coursework-Week3 #526
Conversation
IrynaLypnyk
changed the title
| <div class="menu_icon"> | ||
| <span></span> | ||
| <span></span> | ||
| <span></span> | ||
| </div> |
There was a problem hiding this comment.
Nice custom hamburger menu icon! 👍
|
Good naming of css classes |
| :root { | ||
| --bg: #f3f0ef; | ||
| --text: #4c4946; | ||
| --yellow: #f7e259; | ||
| --yellowLight: #fff9de; | ||
| --ff-body: 'Open Sans', sans-serif; | ||
| --ff-secondary: 'Gwendolyn', cursive; | ||
| } |
| <div class="wrapper"> | ||
| <header class="header"> | ||
| <nav class="menu"> | ||
| <div class="menu_icon"> |
There was a problem hiding this comment.
For accessibility, you should probably make this a <button>. That way screen readers identify it as such.
|
|
||
| .menu_item a { | ||
| text-transform: uppercase; | ||
| font-weight: 500; |
| } | ||
|
|
||
| nav a { | ||
| text-decoration: none; |
There was a problem hiding this comment.
This can be confusing for some users with low vision or just less comfortable with modern trends, especially coupled with the removal of the hyperlink blue text. Users may not realize these are links if they don't look any different from the rest of the page.
| position: relative; | ||
| } | ||
|
|
||
| .menu_icon:hover{ |
There was a problem hiding this comment.
Because you're controlling the cursor, you can simplify this to remove the hover state. It won't apply in any other state anyway
| .menu_icon:hover{ | |
| .menu_icon { |
| </section> | ||
| <section class="gallery content"> | ||
| <ul class="gallery_list"> | ||
| <li class="gallery_item"> |
There was a problem hiding this comment.
For all your lists, you can make the css less brittle and html more terse by removing the class on the items themselves and just select the immediate list items.
.gallery_list li {
If you are particularly concerned about nested lists then
.gallery_list > li {
Should guard the drop-down should guard you against that.
| } | ||
|
|
||
| .footer { | ||
| border-top: 1px solid var(--yellow); |
There was a problem hiding this comment.
This is a very fine line and may be difficult for users with low vision to see, especially with such a similar background color.
This is a really handy tool for checking if foreground-background color contrast is sufficient for all users to perceive.
| .wrapper { | ||
| grid-template-areas: | ||
| "welcome" | ||
| "header" |
| text-align: center; | ||
| justify-content: center; | ||
| max-width: 700px; |
| flex-grow: 1; | ||
| } | ||
|
|
||
| .menu_icon{ |
There was a problem hiding this comment.
As you go to implement this button later on, consider looking up focus traps. For users that rely on keyboards to navigate, it's usually recommended to ensure they can't navigate away while inside the an open modal, like a menu list. An easy way of testing, at least partially, is to observe what happens as you tab around the page. When "inside" a popup, you shouldn't be able to leave and interact with other things unless you close the popup first. This will require JS and be hard to get right.
| .gallery_item:hover { | ||
| transform: scale(1); | ||
| transition: transform 0.3s ease-out; | ||
| } | ||
|
|
||
| .gallery_item:hover { | ||
| transform: scale(1.1); | ||
| } |
|
Sorry, I just realized I did not follow the |
Volunteers: Are you marking this coursework? You can find a guide on how to mark this coursework in
HOW_TO_MARK.mdin the root of this repositoryYour Details
Homework Details
Notes
What did you find easy?
What did you find hard?
What do you still not understand?
Any other notes?