Responsive Hamburger Menu #169
Conversation
mondeja
added
design
jorgeamadosoria
dismissed
mondeja’s stale review
All changes requested were put in
There was a problem hiding this comment.
Hi @jorgeamadosoria. Are you still interested in this PR? I guess that the functionality relative to the input is currently broken.
| rect(width='100', height='20') | ||
| rect(y='30', width='100', height='20') | ||
| rect(y='60', width='100', height='20') | ||
| input.hidden(type='checkbox') |
There was a problem hiding this comment.
What is doing this input? It is not needed, I think 🤔
| #menu-Toggle | ||
| svg(viewBox='0 0 100 80', width='40', height='40') |
There was a problem hiding this comment.
We reallly need a wrapper container for the SVG? I think that the SVG can be the container itself. Also, the identifier breaks our style, it shouldn't be capitalized: menu-toggle.
| .addEventListener('click', (e) => | ||
| domUtils.toggleClass(document.querySelector('#menu'), 'hidden-gt-768px'), |
There was a problem hiding this comment.
The event is not needed here.
- .addEventListener('click', (e) =>
+ .addEventListener('click', () =>| @media (hover: hover) { | ||
| .grid-item__link:hover { | ||
| opacity: 1; | ||
| text-decoration: underline; | ||
| } | ||
|
|
||
| .copy-button:not(:disabled):hover::before { | ||
| cursor: pointer; | ||
| display: block; | ||
| z-index: 1; | ||
| } | ||
| } |
There was a problem hiding this comment.
Why do you've moved all the @media to the end? I think that this confuses more than clarifies.
| #menu-Toggle input { | ||
| display: block; | ||
| width: 40px; | ||
| height: 32px; | ||
| position: absolute; | ||
| top: -7px; | ||
| left: -5px; | ||
|
|
||
| cursor: pointer; | ||
|
|
||
| opacity: 0; /* hide this */ | ||
| z-index: 2; /* and place it over the hamburger */ | ||
|
|
||
| -webkit-touch-callout: none; | ||
| } |
There was a problem hiding this comment.
As before, I don't know that is this.
| #menu-Toggle span:first-child { | ||
| transform-origin: 0% 0%; | ||
| } | ||
|
|
||
| #menu-Toggle span:nth-last-child(2) { | ||
| transform-origin: 0% 100%; | ||
| } | ||
|
|
||
| /* | ||
| * Transform all the slices of hamburger | ||
| * into a crossmark. | ||
| */ | ||
| #menu-Toggle input:checked ~ span { | ||
| opacity: 1; | ||
| transform: rotate(45deg) translate(-2px, -1px); | ||
| background: #232323; | ||
| } | ||
|
|
||
| /* | ||
| * But let's hide the middle one. | ||
| */ | ||
| #menu-Toggle input:checked ~ span:nth-last-child(3) { | ||
| opacity: 0; | ||
| transform: rotate(0deg) scale(0.2, 0.2); | ||
| } | ||
|
|
||
| /* | ||
| * Ohyeah and the last one should go the other direction | ||
| */ | ||
| #menu-Toggle input:checked ~ span:nth-last-child(2) { | ||
| transform: rotate(-45deg) translate(0, -1px); | ||
| } |
|
I'm closing this due to inactivity and very deep conflicts between branches. |
mondeja
added
the
abandoned
Cutoff for the hamburger menu is 768px. Could be smaller but the amount and size of the icons is too much at 768px to keep in one line.
The cutoff could be moved to 648 or smaller if necessary, if the icon sizes are changed for 768px. I like it as it is, but I'm not married to it.
A new menu.js file is introduced, to initialize the burger icon click. I think it's overkill, but that's the architecture, nothing to do about it.
The stylesheet is refactored to contain all the classes for a particular size together, order by size in descendent order. That way smaller resolution screens will overwrite bigger ones, as expected, and all classes for a particular resolution are lumped together.
A new media query is added, with min-width: 768px. This media query contains the hover and focus effects for the icons in the menu, which are not needed in the responsive menu since there are no icons there.
Several new CSS classes are introduced as well as new ids for menu elements.