Conversation
Media queries are added and specific css changes are made (e.g. new flexbox layout and image size). A new meta tag that specifies the viewport and JS code for responsiveness is added.
Turnt flex-direction from row to column and specified size for .text and img in relation with the .card container.
Changed the partner logo's grid to flexbox and made adjustments to the width.
There was a problem hiding this comment.
Changes
The previous navbar toggler required the user to press the button twice because the specific css code would be added to the html tag first until if-statement can make sense of it. Now the toggle button toggles between two classes and only requires on button push. Additionally, the specific event page is now mobile friendly.
MTCoster
left a comment
MTCoster
left a comment
There was a problem hiding this comment.
Mostly minor things, see individual comments for details. I stopped commenting for formatting fixes, but follow through and fix all of them please.
_includes/page/header.html
Outdated
| </a> | ||
| </div> | ||
| <button id="navbar-toggler" type="button"> | ||
| <i class="fas fa-bars"></i> |
_includes/page/links.html
Outdated
| {% endcomment %} | ||
| <meta charset="utf-8"> | ||
| <meta name="viewport" content="width=960"> | ||
| <!-- <meta name="viewport" content="width=960"> --> |
There was a problem hiding this comment.
Don’t leave comments in code - this is why we have a version history!
| {%- comment %} | ||
| Site resources | ||
| {% endcomment %} | ||
| <link rel="stylesheet" href="https://use.fontawesome.com/releases/v5.6.3/css/all.css" integrity="sha384-UHRtZLI+pbxtHCWp1t77Bi1L4ZtiqrqD80Kn4Z8NTSRyMA2Fd33n5dQ8lWUE00s/" crossorigin="anonymous"> |
There was a problem hiding this comment.
This seems like a poor choice - adding 54kB to every page load just for a single close icon
_sass/_cards.scss
Outdated
| } | ||
| } | ||
|
|
||
| @media (max-width: 960px){ |
_sass/_cards.scss
Outdated
| } | ||
|
|
||
| @media (max-width: 960px){ | ||
| margin: 17px 0px; |
There was a problem hiding this comment.
Zero sizes in CSS should have no units
_sass/_header.scss
Outdated
| border: transparent; | ||
| font-size: 22px; | ||
| position: absolute; | ||
| right: 30px; |
There was a problem hiding this comment.
These dimensions should probably be em as well
_sass/_header.scss
Outdated
| } | ||
| } | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
Files should end with a newline
assets/js/init.js
Outdated
| const content = document.getElementById('content'); | ||
| const padding = header.offsetHeight - threshold; | ||
| const toggler = document.getElementById('navbar-toggler'); | ||
| const nav = document.getElementsByClassName('nav')[0]; |
There was a problem hiding this comment.
Don’t pad like this. And don’t use getElementsByClassName() - add an id to the element instead
assets/js/init.js
Outdated
|
|
||
| // Toggle between display none and block for mobile navbar | ||
| toggler.addEventListener('click', () => { | ||
| if(nav.className == "nav"){ |
There was a problem hiding this comment.
Never use double quotes (") in JS, always single (')
assets/js/init.js
Outdated
|
|
||
| // Toggle between display none and block for mobile navbar | ||
| toggler.addEventListener('click', () => { | ||
| if(nav.className == "nav"){ |
There was a problem hiding this comment.
This mechanism seems rocky, add and remove a nav-small class on top of the existing nav class to prevent duplication of CSS
2 participants
Changes
A number of new media queries have been added to enable a mobile-friendly site. Mostly new flexbox settings were added in these queries to ensure that the site's content is distributed throughout the small display. Additionally, a button was added that is shown when the display's width is small enough which shows the navbar as a dropdown menu.