Skip to content
Open
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions _includes/page/header.html
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,9 @@
<img src="{{ "/assets/img/brand/header-logo.svg" | relative_url }}" alt="TEDxWarwick">
</a>
</div>
<button id="navbar-toggler" type="button">
<i class="fas fa-bars"></i>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrong indent

</button>
<div class="nav">
<div class="cta">
{% include components/cta.html %}
Expand Down
4 changes: 3 additions & 1 deletion _includes/page/links.html
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,8 @@
Metadata
{% endcomment %}
<meta charset="utf-8">
<meta name="viewport" content="width=960">
<!-- <meta name="viewport" content="width=960"> -->
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don’t leave comments in code - this is why we have a version history!

<meta name="viewport" content="width=device-width, initial-scale=1">
<link rel="canonical" href="{{ site.url }}{{ page.url | relative_url }}">
<meta name="description" content="{{ page.description | strip | default: "An annual conference on the University of Warwick campus, bringing inspiring speakers and ground-breaking ideas to the West Midlands and beyond." }}">
{%- comment %}
Expand Down Expand Up @@ -34,6 +35,7 @@
{%- 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">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like a poor choice - adding 54kB to every page load just for a single close icon

<link type="text/css" rel="prefetch" href="//fonts.googleapis.com/css?family=PT+Serif:400,400i,700,700i|Source+Sans+Pro:600,600i|Fira+Mono:400,700">
<link type="text/css" rel="stylesheet" href="{{ "/assets/css/style.css" | relative_url }}">
<script type="application/javascript" src="{{ "/assets/js/init.js" | relative_url }}"></script>
Expand Down
21 changes: 21 additions & 0 deletions _sass/_event.scss
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,9 @@
@include elevate(8);
}
}
.container{
padding: 0px 0px;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More zero values

}
}

.card.event-card {
Expand All @@ -24,6 +27,17 @@
img {
flex: 0 0 ($card-height * (1920 / 1080));
}

@media (max-width: 960px){
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spaces & blank lines in this block

& {
flex-direction: column;
height: 100%;
}
img {
flex: auto;
}

}
}

.speakers {
Expand All @@ -32,4 +46,11 @@
grid-gap: 40px;
justify-content: space-between;
margin: 1rem 0;

@media (max-width: 960px){
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spaces & zero units

& {
display: flex;
}

}
}
67 changes: 66 additions & 1 deletion _sass/_header.scss
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,23 @@ header {
transition: hover-transition(border-bottom),
hover-transition(box-shadow);


Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extraneous blank line

@media (max-width: 960px){
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Space

& {
height: inherit;
}
}

.container {
height: 100%;
display: flex;
flex-direction: row;

@media (max-width: 960px){
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Space

& {
flex-direction: column;
}
}
}

.brand {
Expand All @@ -28,6 +41,33 @@ header {
// MS Edge doesn't like unbounded SVG images.
// This value was derived empirically using Chrome 69.
max-width: 501px;

@media (max-width: 960px){
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Space

& {
height: 38px;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where does this value come from? Just curious

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just experimented with different sizes and thought this fits the navbar best.

margin: 12px 0px;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Zero units

max-width: initial;
}
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extraneous blank line

}
}

button {
border: transparent;
font-size: 22px;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope nope nope - use em for fonts to keep the whole site relative

position: absolute;
right: 30px;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These dimensions should probably be em as well

top: 18px;
padding: 0px;
display: none;
cursor: pointer;
background-color: transparent;

@media (max-width: 960px){
& {
display: block;
}
}
}

Expand All @@ -36,8 +76,16 @@ header {
display: flex;
flex-direction: column;
margin-left: 2em;

@media (max-width: 960px){
& {
margin-left: 0px;
display: none;
}
}
}


.cta {
flex: 1;
text-align: right;
Expand All @@ -47,17 +95,34 @@ header {
flex-direction: column;
justify-content: center;
padding-right: 1rem;

@media (max-width: 960px){
text-align: left;
}
}

nav {
@include font(menu);

text-align: right;

@media (max-width: 960px){
& {
text-align: left;
margin-left: 0em;
}
}

ul {
display: flex;
flex-direction: row;
justify-content: flex-end;

@media (max-width: 960px){
& {
flex-direction: column;
}
}
}

li {
Expand Down Expand Up @@ -141,4 +206,4 @@ header {
padding-right: 0;
}
}
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Files should end with a newline

4 changes: 4 additions & 0 deletions _sass/_home.scss
Original file line number Diff line number Diff line change
Expand Up @@ -42,5 +42,9 @@
display: flex;
flex-direction: column;
justify-content: space-between;

@media (max-width: 960px){
padding: 0px 20px;
}
}
}
10 changes: 10 additions & 0 deletions _sass/_layout.scss
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,10 @@ header, main, footer {
.container {
width: 960px;
margin: 0 auto;

@media (max-width: 960px){
padding: 0px 20px;
}
}

section {
Expand All @@ -32,3 +36,9 @@ main {
footer {
margin-top: 5em;
}

@media (max-width: 960px){
.container {
width: 100%;
}
}
19 changes: 18 additions & 1 deletion _sass/_partners.scss
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,19 @@
img {
width: auto;
margin: 10px;

@media (max-width: 960px){
width: max-content;
}
}

@media (max-width: 960px){
display: flex;
flex-direction: row;
justify-content: center;
margin: 11px 0px;
}

}

.partners {
Expand All @@ -13,6 +25,11 @@
grid-gap: 40px;
justify-content: space-between;
margin: 1rem 0;

@media (max-width: 960px){
display: flex;
flex-direction: column;
}
}

.partner-large {
Expand All @@ -23,4 +40,4 @@
height: 9rem;
margin: 0.25rem 0.5rem 0.25rem 0;
}
}
}
12 changes: 12 additions & 0 deletions _sass/_text.scss
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,10 @@ p {
letter-spacing: rem(-0.5);
margin: line-steps(4) 0;
font-weight: 300;

@media (max-width: 960px){
font-size: rem(35)
}
}

@mixin h3 {
Expand All @@ -59,6 +63,10 @@ p {
letter-spacing: 0;
margin: line-steps(4) 0;
font-weight: 400;

@media (max-width: 960px){
font-size: rem(30)
}
}

@mixin h4 {
Expand All @@ -69,6 +77,10 @@ p {
letter-spacing: rem(0.25);
margin: line-steps(3) 0;
font-weight: 400;

@media (max-width: 960px){
font-size: rem(30)
}
}

@mixin h5 {
Expand Down
17 changes: 15 additions & 2 deletions assets/js/init.js
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,8 @@ const x = {
const threshold = document.querySelector('header .cta').clientHeight;
const content = document.getElementById('content');
const padding = header.offsetHeight - threshold;
const toggler = document.getElementById('navbar-toggler');
const nav = document.getElementsByClassName('nav')[0];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don’t pad like this. And don’t use getElementsByClassName() - add an id to the element instead


x.extend({ headerOverlap: threshold });

Expand All @@ -80,7 +82,7 @@ const x = {
let wasSmallHeader = false;

window.addEventListener('scroll', e => {
smallHeader = window.scrollY >= threshold;
smallHeader = window.scrollY >= threshold && window.outerWidth > 960;

if (!ticking) {
window.requestAnimationFrame(() => {
Expand All @@ -104,6 +106,17 @@ const x = {
ticking = true;
}
});

// Toggle between display none and block for mobile navbar
toggler.addEventListener('click', () => {
if(nav.className == "nav"){
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Never use double quotes (") in JS, always single (')

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This mechanism seems rocky, add and remove a nav-small class on top of the existing nav class to prevent duplication of CSS

nav.className = "nav-small";
} else {
nav.className = "nav";
}
})


},
],
};
};