Learning sections #221
Learning sections #221
Conversation
|
Also want to address #199 |
|
thanks for doing this PR!
is this missing some styles? |
|
Yeah WIP, styling was going to be the next step :) |
|
ok, sweet - that's good. I actually prefer WIP like this. thanks 👍 |
|
lmk when this ready for another review |
| <div> | ||
| <div> | ||
| <a itemprop="url" href="{{ url }}" class="no-underline"> | ||
| <img src="{{ thumb }}"/> |
There was a problem hiding this comment.
can you add alt="{{ name }}"?
| @@ -46,8 +38,25 @@ | |||
| </div> | |||
| </section> | |||
|
|
|||
| <section id="learn" class="section"> | |||
| <h1>Learn about WebVR</h1> | |||
There was a problem hiding this comment.
maybe add a <a href="#learn">…</a>?
| "author": "Arturo Paracuellos", | ||
| "author_url": "unboring.net", | ||
| "thumbnail": "articles/progressive-webvr/thumbnail.png", | ||
| "date_published": "5-April-2017" |
There was a problem hiding this comment.
I'd change this to April 5, 2017 (to be consistent with the dates elsewhere)
| @@ -0,0 +1,20 @@ | |||
| { | |||
| "progressive-webvr": { | |||
| "name": "Progressive Enhancement with WebVR.", | |||
| { | ||
| "progressive-webvr": { | ||
| "name": "Progressive Enhancement with WebVR.", | ||
| "description": "Arturo Paracuellos, the creator of Puzzle Rain and other notable WebVR experiences, runs us through how he made a WebVR experience for weather.com that progressively enhances from mobile phones all the way up to full VR systems like the HTC VIVE.", |
| "date_published": "5-April-2017" | ||
| }, | ||
| "tools": { | ||
| "name": "Tools for creating WebVR content.", |
| }, | ||
| "tools": { | ||
| "name": "Tools for creating WebVR content.", | ||
| "description": "See all the options available for creating WebVR content", |
There was a problem hiding this comment.
I'd add a period (to be consistent with the description above)
| "description": "See all the options available for creating WebVR content", | ||
| "url": "tools.html", | ||
| "author": "Casey Yee", | ||
| "author_url": "twitter.com/whoyee", |
| "url": "tools.html", | ||
| "author": "Casey Yee", | ||
| "author_url": "twitter.com/whoyee", | ||
| "thumbnail": "articles/tools/thumbnail.jpg", |
| "author": "Casey Yee", | ||
| "author_url": "twitter.com/whoyee", | ||
| "thumbnail": "articles/tools/thumbnail.jpg", | ||
| "date_published": "9-May-2017" |
There was a problem hiding this comment.
can you change to May 9, 2017?
| @@ -288,6 +288,7 @@ h1.page-heading { | |||
| .notifications { | |||
| display: flex; | |||
| flex-direction: column-reverse; | |||
| margin-bottom: 1.5rem; | |||
There was a problem hiding this comment.
how does this look on mobile on the homepage?
| @@ -0,0 +1,36 @@ | |||
| {% from '_helpers.html' import author_item, browsers, site_title %} | |||
| {% set browser = browsers.servo %} | |||
There was a problem hiding this comment.
remove this and the line below
| @@ -0,0 +1,36 @@ | |||
| {% from '_helpers.html' import author_item, browsers, site_title %} | |||
There was a problem hiding this comment.
you can remove author_item, which seems to be unused
| {% set browser = browsers.servo %} | ||
| {% set page = 'servo.html' %} | ||
| <!doctype html> | ||
| <html lang="en" data-layout="secondary" data-browser="{{ browser.slug }}"> |
There was a problem hiding this comment.
you can remove the data-browser attribute
There was a problem hiding this comment.
can you change to data-layout="secondary tools"?
| <html lang="en" data-layout="secondary" data-browser="{{ browser.slug }}"> | ||
| <head> | ||
| {% include '_head.html' %} | ||
| <title>Tools</title> |
There was a problem hiding this comment.
can you change to add the WebVR Rocks text to the title?
<title>Tools • {{ site_title }}</title>
| <div class="container section"> | ||
| {% include '_logo.html' %} | ||
|
|
||
| <a class="page-heading-link" href="{{ browser.about }}"> |
There was a problem hiding this comment.
can you change this to href="/tools?
There was a problem hiding this comment.
you could replace this with something like this instead:
<a class="page-heading-link" href="/tools">
<h1 class="page-heading">Tools</h1>
</a>
<p class="browser-intro page-intro">Creating WebVR content</p>
| <main id="main" class="main" role="main"> | ||
| <div class="container"> | ||
|
|
||
| <section class="section"> |
| <section id="browsers" class="section browsers" data-section="browsers"> | ||
| <h1><a href="#browsers">WebVR Browsers</a></h1> | ||
| <h1><a href="#browsers">Supported Browsers</a></h1> |
There was a problem hiding this comment.
you can lowercase Browsers: browsers
| <h1><a href="#browsers">WebVR Browsers</a></h1> | ||
| <h1><a href="#browsers">Supported Browsers</a></h1> | ||
|
|
||
| <div class="notifications" id="notifications"> |
There was a problem hiding this comment.
do you think we need to show the Notifications?
Adds section for article resources as per #217