-
Notifications
You must be signed in to change notification settings - Fork 11
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add styling for speakers overview and details pages #82
Conversation
contents/css/_layout.css
Outdated
} | ||
.grid-item-speaker:nth-child(2n+2) { | ||
grid-column: span 3 / -4; | ||
margin-top: 14rem; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure whether this would do the trick for what the design team had in mind… Let’s test it and decide what to do about this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with some comments/questions
<figure class="ma0 speaker-picture"> | ||
<img src="{{ contents.images.speaker[speaker.image.filename].url }}" | ||
alt="Portrait photo of {{ speaker.firstname }} {{ speaker.lastname }}" | ||
data-aspect-ratio="{{ (speaker.image.height / speaker.image.width) * 100 }}%"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was just my example for what to do if we wanted to make the images responsive. If we go with squares, this can just be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True that, will remove it. :)
<ul> | ||
{% if speaker.links.twitter %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This part isn't designed yet, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No it's not. I’ll try to talk to Silke or Nicolas about this today.
design: looks good except some details, added it to the checklist above. |
Thanks @Kriesse! |
@@ -31,7 +31,7 @@ p, ul, ol, blockquote { | |||
font-size: 1.375rem; | |||
} | |||
@media (--medium) { | |||
font-size: 1.5rem; | |||
//font-size: 1.5rem; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd love to re-discuss this 1.5 rem, which resizes all fonts for screens larger than medium. It seems to be intentionally in order to resize the font. Is it necessary? Design-wise 22 px, in my opinion, work more harmonic with the rest of the content and we're encountering spacing problems on pages like the speaker page...
Hey y'all What I like to tackle (or delegate, depending on my next weeks' office workload):
|
Merging this, so we can iterate with smaller change sets. |
Related to #44.
First part of #60.
TODO: