[LAND-SEE] - Add mocks, images, home template, and navigation #596

afilbert · 2017-08-20T01:20:45Z

Initial PR for Land + See templates. This adds the home template, references stories template (which will be added in subsequent PRs), and establishes the main, sidebar, and footer nav.

Please see mockups in /mocks/land-see, categorized based off of approved design mockups:

https://projects.invisionapp.com/share/5DCXJEX3P#/screens/247500921_Home-Desktop-V5

Committed images needed to build out the home template, along with both .png and .svg samples of structural images. I plan on removing redundant image types as PRs are approved, but wanted to commit both for now. As mentioned on our initial dev conference call, @camelburrito, I've embedded the waves .svgs as tiling background images in css. That's kinda pushing the size of the css file already, so we may need to consider an alternative approach.

I'm working with Grain & Mortar as a consultant, so I signed an individual CLA. Please let me know if that's not sufficient.

camelburrito · 2017-08-23T07:53:23Z

img/land-see/structure/accent-green-up.svg

@@ -0,0 +1,13 @@
+<?xml version="1.0" encoding="utf-8"?>


optimize using svgo tool

Thanks for the svgo suggestion, @camelburrito. I went ahead and committed the optimized svgs.

camelburrito

Took a quick stab, most of the changes seem okay. Can you quickly explain why you were not able reuse footer and nav. Also can you provide a staging url for this PR?

afilbert · 2017-08-24T01:27:20Z

For sure, here's the staging URL: https://landseeampstart.firebaseapp.com/templates/land-see/land-see.amp

I felt that our theme styling was opinionated enough that it warranted creating custom partials as opposed to modifying the base components with new rules/markup.

There were both Basecss classes and functional assumptions that the base components implemented, and that weren't editable by passing JSON variables, that didn't align with the Land + See theme design. Major items being:

Dropdowns/accordions in the sidebar being expandable vs our design featuring a fixed subnav for Stories
The simple search icon as opposed to the input on main nav, and the need for it to persist at all breakpoints
Specific layout of the footer, including how it behaves responsively when scaling down

Please let me know your thoughts and suggestions.

afilbert · 2017-09-04T22:45:57Z

I looked into using the ampstart navbar component some more and realized that {{#fixeditems}} could be used for the search icon. The problem I ran into there is that I'd like to use the font-awesome icon instead of passing in an svg. So I'm continuing to use my custom partial, but updated the "search" icon to be more customizable via JSON variables.

camelburrito

Overall Things look great, Just minor CSS nits

I commented on how to re-use existing components , happy to get on a call if things are not clear.
I really liked how you filled out the variables . This makes existing copypasteable components work on your template !

Thank you!

camelburrito · 2017-09-08T17:09:19Z

css/templates/land-see/page.css

+h2,
+.h2 {
+  letter-spacing: 0;
+}


merge these rules into one

camelburrito · 2017-09-08T17:12:19Z

css/templates/land-see/page.css

+  left: 0;
+  width: 100%;
+  height: 1px;
+  background: #efe5e3;


why not just add border-bottom to the hr?

I originally wanted to control the width of the border, but realized I could just use 100% width due to container margins to achieve the same outcome. You're right, that could've been simplified. I did, however, end up reusing that class to add a border to the base ampstart nav component in the sidebar.

one small nit: instead of width 100% use right:0;

camelburrito · 2017-09-08T17:13:21Z

css/templates/land-see/page.css

+  background: #efe5e3;
+}
+
+.fa {


what is fa? - lets not create more undocumented utility classes.

That's a font-awesome class that I'm overriding. Eliminating that in my next commits.

camelburrito · 2017-09-08T17:36:18Z

templates/land-see/partials/footer.snip.html

+
+{{#footer}}
+  <!-- Start Footer -->
+  <footer class="ampstart-footer flex flex-column items-center {{#spacing-classes}}{{.}}{{/spacing-classes}}{{^spacing-classes}}px3{{/spacing-classes}}">


I reviewed the footer partial and compared it against the existing footer component

It is possible to fully reuse the footer partial,

ampstart-footer ampstart-footer-nav ampstart-social-follow classes should be more than enough to do all this with flexboxes (and more CSS as needed.)

To reorder DOM - you can use flexbox ordering

retain the existing copyright and you can add a new TOC variable if you need in the footer component

camelburrito · 2017-09-08T18:28:26Z

templates/land-see/partials/nav.snip.html

+-->
+}}
+
+{{#navbar}}


Again same feedback as above - we should try and use the components code as much as possible

I looked into using the ampstart navbar component some more and realized that {{#fixeditems}} could be used for the search icon. The problem I ran into there is that I'd like to use the font-awesome icon instead of passing in an svg. So I'm continuing to use my custom partial, but updated the "search" icon to be more customizable via JSON variables.

You are free to modify the components library to fit in anything that does not already have a place for it in there.

Also make sure the stuff you use has an appropriate distribution license , Apache-2 or CC0 I guess for images. Not sure if font awesome is re-distributable

Eliminating font-awesome in next commit. That will solve a couple issues. I'll now just pass in our svg to existing {{#fixeditems}}, and we won't need to rely on any font-awesome dependencies.

afilbert · 2017-09-09T16:37:44Z

Thanks for the pointers, @camelburrito. My approach considered the tradeoff of custom partials/markup in exchange for reducing the amount of CSS needed to override the base components sufficiently enough to suit our theme. Sounds like more CSS is acceptable. Going to attempt reusing the base components with minimal modification.

afilbert · 2017-09-10T19:26:55Z

Deployed latest to staging for your review, @camelburrito: https://landseeampstart.firebaseapp.com/templates/land-see/land-see.amp

Components mods:

{{initial-state}} on dropdown-inline.snip.html
{{terms}} on footer.snip.html

Let me know if you need any additional changes. Thanks!

camelburrito

Code Looks Good, this is good to go after the 3 minor stuff are fixed.

camelburrito · 2017-09-10T19:49:15Z

css/templates/land-see/page.css

+  left: 0;
+  width: 100%;
+  height: 1px;
+  background: #efe5e3;


one small nit: instead of width 100% use right:0;

camelburrito · 2017-09-10T20:12:26Z

templates/land-see/land-see.amp.json

+    },
+    "copyright": {
+      "text": "© Land + See, 2017 • ",
+      "terms":


"tos": { "classes": "", "link": { "url":"#", "text": "text" } }

This will line it up close to existing stuff and make it extensible

camelburrito · 2017-09-10T20:13:45Z

css/templates/land-see/page.css

+  letter-spacing: .1rem;
+}
+
+.sidebar-nav-item {


Can you namespace this class so that it is sepcific to your template (i think you use land-see)

afilbert · 2017-09-10T21:57:06Z

Minor changes fixed, staging deployed @camelburrito!

camelburrito · 2017-09-10T22:06:13Z

Congrats on your First PR!

afilbert · 2017-09-10T22:23:53Z

Thanks, @camelburrito!

…ject#596) * add mocks, customize components, copy css [wip] * add images, tweak css * primary, sidebar, footer nav [wip] * images tweaks * wrap up initial nav work * add land-see, may need alternative to svg waves * remove .ai files from repo * move image into correct folder * optimize svgs with svgo * make icon customizable * make target of search _blank * refactor nav to use ampstart base components * add search icon * trim whitespace ✂️ * resolve PR requests * build updated CSS_SIZES

camelburrito self-requested a review August 23, 2017 07:52

camelburrito reviewed Aug 23, 2017

View reviewed changes

camelburrito requested changes Sep 8, 2017

View reviewed changes

afilbert added 14 commits September 10, 2017 13:51

add mocks, customize components, copy css [wip]

b6ad9b1

add images, tweak css

9956f62

primary, sidebar, footer nav [wip]

20bde7c

images tweaks

2a61dd9

wrap up initial nav work

40a89fa

add land-see, may need alternative to svg waves

92e20a2

remove .ai files from repo

48d1534

move image into correct folder

a534e96

optimize svgs with svgo

e030094

make icon customizable

e2c324c

make target of search _blank

4725951

refactor nav to use ampstart base components

b510fc8

add search icon

ba7a7ff

trim whitespace ✂️

77fdf61

afilbert force-pushed the master branch from b4f4659 to 77fdf61 Compare September 10, 2017 19:02

camelburrito requested changes Sep 10, 2017

View reviewed changes

afilbert added 2 commits September 10, 2017 16:35

resolve PR requests

dc07ae1

build updated CSS_SIZES

ac25c56

camelburrito approved these changes Sep 10, 2017

View reviewed changes

camelburrito merged commit 0cd9662 into ampproject:master Sep 10, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[LAND-SEE] - Add mocks, images, home template, and navigation #596

[LAND-SEE] - Add mocks, images, home template, and navigation #596

afilbert commented Aug 20, 2017

camelburrito Aug 23, 2017

afilbert Aug 24, 2017

camelburrito left a comment •

edited

Loading

afilbert commented Aug 24, 2017

afilbert commented Sep 4, 2017

camelburrito left a comment

camelburrito Sep 8, 2017

camelburrito Sep 8, 2017

afilbert Sep 10, 2017

camelburrito Sep 10, 2017

camelburrito Sep 8, 2017

afilbert Sep 9, 2017

camelburrito Sep 8, 2017

camelburrito Sep 8, 2017

afilbert Sep 9, 2017

afilbert commented Sep 9, 2017

afilbert commented Sep 10, 2017

camelburrito left a comment

camelburrito Sep 10, 2017

camelburrito Sep 10, 2017

camelburrito Sep 10, 2017

afilbert commented Sep 10, 2017

camelburrito commented Sep 10, 2017

afilbert commented Sep 10, 2017

[LAND-SEE] - Add mocks, images, home template, and navigation #596

[LAND-SEE] - Add mocks, images, home template, and navigation #596

Conversation

afilbert commented Aug 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

camelburrito left a comment • edited Loading

Choose a reason for hiding this comment

afilbert commented Aug 24, 2017

afilbert commented Sep 4, 2017

camelburrito left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afilbert commented Sep 9, 2017

afilbert commented Sep 10, 2017

camelburrito left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afilbert commented Sep 10, 2017

camelburrito commented Sep 10, 2017

afilbert commented Sep 10, 2017

camelburrito left a comment •

edited

Loading