Add article and sidebar menu organisms and main/sidebar template #86

craigcook · 2018-05-02T21:19:14Z

This introduces a bunch of new bits and slightly updates a few existing bits. It looks bigger than it is, but it does have lots of small changes going on.

New "article" organism.
New "sidebar menu" organism.
New "main with sidebar" template.
New article demo page.
New main-with-sidebar template demo page.
New bi-directional mixin (swiped from MDN).
New grid mixins that could maybe be reduced to just tokens for the widths.
Updated unordered list styling.
Renamed table styling.
Adds tokens for media queries, content sizes, and spacing. These will eventually move to the tokens package but are included here for the moment.

Staged at https://demo2--mozilla-protocol.netlify.com/

craigcook · 2018-05-02T21:41:33Z

src/assets/js/protocol/protocol-sidemenu.js

+    var menuToggle = document.querySelector('.mzp-js-toggle');
+
+    // Add class to reflect javascript availability for CSS
+    $doc.className = $doc.className.replace(/\bno-js\b/, 'js');


This is may be redundant here and would collide with any other scripts doing the same thing. We should consider some kind of minimal global JS as part of protocol-core, even if all it does is set this class.

OR we can totally rethink it and not rely on this root-level script-is-working indicator. Each component can make its own check and set a class for a style hook for that component, but it sure is handy to have one hook in one place.

I think ideally this piece of JS should be run in the <head>, so as to avoid content flicker. So making this a piece of global JS probably makes sense?

I added a protocol-base.js that so far does nothing but this JS detection class but could possibly include other essentials down the road.

alexgibson

A few comments / suggestions but this looks great. Nice work!

alexgibson · 2018-05-03T09:26:56Z

src/assets/js/protocol/protocol-sidemenu.js

+    var menuToggle = document.querySelector('.mzp-js-toggle');
+
+    // Add class to reflect javascript availability for CSS
+    $doc.className = $doc.className.replace(/\bno-js\b/, 'js');


I think ideally this piece of JS should be run in the <head>, so as to avoid content flicker. So making this a piece of global JS probably makes sense?

alexgibson · 2018-05-03T09:33:04Z

src/assets/js/protocol/protocol-sidemenu.js

+(function() {
+    'use strict';
+
+    var $doc = document.documentElement;


Nit: rename variable to doc? (the $ is a carryover from jQuery days gone by)

alexgibson · 2018-05-03T09:36:14Z

src/assets/js/protocol/protocol-sidemenu.js

+    // Toggle the sidebar menu
+    menuToggle.addEventListener('click', function(e) {
+        e.preventDefault();
+        menu.classList.toggle('mzp-is-active');


This toggle on small screens here is currently a <section> element, which isn't ideal for a11y. I'd suggest adding some aria roles here? Using aria-role="button" would give context that the element is clickable, but we could also add arai-controls and then aria-expanded to the menu itself and toggle the state?

alexgibson · 2018-05-03T09:39:00Z

src/assets/sass/demos/article.scss

@@ -0,0 +1,567 @@
+// Define paths
+$font-path:  '/assets/protocol/protocol/fonts';


These paths are already set in docs/protocol.scss along with the imports below. Could we import that here (in the same way the type demo does) to avoid repeating?

alexgibson · 2018-05-03T09:43:20Z

src/assets/sass/demos/article.scss

+    '../protocol/components/sidebar-menu';
+
+
+// Article links... is this a component?


I've been struggling with this too, and settled on that if something is an organism it should probably be classed as a component. But then we have two names for the same thing which seems... confusing. Maybe something we could bikeshed on later?

This was a holdover from the security page this was originally based on, and on that page at least this set of links is a one-off thing. We'll have to figure out what to do about that when we're actually reskinning the security pages in bedrock, but I think for now I can just delete this bit here since we're not using it in the demo.

alexgibson · 2018-05-03T09:57:52Z

src/assets/sass/protocol/base/elements/_lists.scss


    li {
        margin-bottom: .25em;
+
+        &:before {
+            content: '•';


Just curious, what's the rationale for using a pseudo element for list styles over allowing the browser to manage it - just for more control over size/spacing?

Yeah, just for size and spacing. We can talk to Vince about it but this was what bulleted lists looked like in the mockup so I tried to match it. I'm happy with browser defaults if the designers are.

alexgibson · 2018-05-03T10:00:03Z

src/assets/sass/protocol/base/elements/_tables.scss

-        text-align: left;
-        padding-bottom: 1px; /* fix to get it to look nice in many browsers */
-        margin-bottom: -1px;
+        @include bidi(((text-align, left, right),));


alexgibson · 2018-05-03T10:01:21Z

src/assets/sass/protocol/components/_article.scss

+.mzp-c-article {
+    max-width: $content-md;
+
+    // Step down the heading levels


The frequency that we seem to do this suggests we probably need to revisit our default heading levels. Maybe something we should open an issue for discussion on?

A revision of the type scale is in the works.

alexgibson · 2018-05-03T10:07:23Z

src/assets/sass/protocol/includes/mixins/_bidi.scss

+    RTL: float: right;
+    A list with four properties overrides the ltr value in rtl locales
+    AND provides a new rtl only property value
+    @include bidi(((margin-right, 20px, margin-left, 0),))


Should this be @include bidi(((margin-right, 10px, margin-left, 0),))?

alexgibson · 2018-05-03T10:20:30Z

src/patterns/organisms/sidebar-menu/sidebar-menu.hbs

+      <li><a href="#">And one more menu item</a></li>
+    </ul>
+  </section>
+</nav>


Include the JS for the side menu here?

<script src="/assets/protocol/protocol/js/protocol-sidemenu.min.js"></script>

alexgibson · 2018-05-04T13:27:05Z

One minor thing:

The concatJS gulp task is joining every JS file in /js/protocol/ together, but we probably don't want protocol-base.js included in that, as it should be separate and in the <head>. Perhaps we want to exclude this file from concatenation?

alexgibson · 2018-05-04T13:28:10Z

src/assets/js/protocol/protocol-sidemenu.js

+    var menu = document.querySelector('.mzp-c-sidemenu');
+    var menuToggle = document.querySelector('.mzp-js-toggle');
+
+    // Add class to reflect javascript availability for CSS


Should we remove this from the sidemenu JS now that we have a separate base JS file?

alexgibson · 2018-05-04T13:33:17Z

This doesn't need to be a blocker, but currently we aren't compiling any CSS files for either the article or side menu, and it's up to the consuming site to compile the sass. This might be ok if it's what we decide we want to do, but it's currently not possible to include article.css (for example) in a page without sites having their own build step.

alexgibson

A couple of suggestions, and an open question about if we should publish the compiled CSS files for these new components. But I think this is just about ready to merge? There are no doubt more things we will continue tweaking in the CSS, but we can always follow up later.

craigcook · 2018-05-04T14:42:49Z

This doesn't need to be a blocker, but currently we aren't compiling any CSS files for either the article or side menu, and it's up to the consuming site to compile the sass. This might be ok if it's what we decide we want to do, but it's currently not possible to include article.css (for example) in a page without sites having their own build step.

Yeah I've been thinking about the best way to publish components without creating one huge monolith. Dozens of individual CSS files isn't any better than one big one (probably worse). For bedrock we'll do our own compilation anyway and just @import the components we need per page, but longer term if we want to offer pre-compiled CSS we'll probably need to make a tool to pick your components, like https://foundation.zurb.com/sites/download.html/#customizeFoundation

In the short term we'll still need to get these components published and consumable as SCSS. Taking more inspiration from what Spark is doing, I think we'll need to make a separate package for protocol-components.

alexgibson · 2018-05-04T14:51:40Z

Yeah I've been thinking about the best way to publish components without creating one huge monolith. Dozens of individual CSS files isn't any better than one big one (probably worse). For bedrock we'll do our own compilation anyway and just @import the components we need per page, but longer term if we want to offer pre-compiled CSS we'll probably need to make a tool to pick your components, like https://foundation.zurb.com/sites/download.html/#customizeFoundation

In the short term we'll still need to get these components published and consumable as SCSS. Taking more inspiration from what Spark is doing, I think we'll need to make a separate package for protocol-components.

In an http2 world, I think making individual CSS files is probably the better solution honestly. Sites will move away from bundling, and monolith CSS/JS files will be considered an anti-pattern.

We could go all the way to publish a separate package a-la protocol-components. But if we provided separate CSS files anyway, there's no real reason we couldn't just use one package still, and people could pick & choose what they use. Just my 2 pence

alexgibson

Couple of things to tidy up in the menu JS, but otherwise r+wc

alexgibson · 2018-05-04T14:58:02Z

src/assets/js/protocol/protocol-sidemenu.js

+            menuExpanded = 'true';
+        }
+        menuMain.setAttribute('aria-expanded', menuExpanded);
+        alert(menuExpanded);


Remove alert

alexgibson · 2018-05-04T15:00:12Z

src/patterns/organisms/sidebar-menu/sidebar-menu.hbs

+---
+
+<nav class="mzp-c-sidemenu">
+  <section class="mzp-c-sidemenu-summary mzp-js-toggle" aria-role="button" aria-controls="">


I think the value for aria-controls should be the ID of the menu?

Should both this and aria-role="button" be set in the JS along with where you set tabindex? They probably don't apply when JS is disabled?

alexgibson · 2018-05-04T15:05:27Z

src/assets/js/protocol/protocol-sidemenu.js

+        menu.classList.toggle('mzp-is-active');
+
+        menuExpanded = menuMain.getAttribute('aria-expanded');
+        if (menuExpanded == 'true') {


You could make this logic a little simpler with something like:

var menuExpanded = menuMain.getAttribute('aria-expanded') == 'true' ? 'false' : 'true';

alexgibson · 2018-05-04T15:07:05Z

src/assets/js/protocol/protocol-sidemenu.js

+    var menu = document.querySelector('.mzp-c-sidemenu');
+    var menuMain = document.querySelector('.mzp-c-sidemenu-main');
+    var menuToggle = document.querySelector('.mzp-js-toggle');
+    var menuExpanded;


I don't think this variable needs to be in this scope for any reason, you can probably just declare it inside the click even listener?

alexgibson

🎉

craigcook · 2018-05-04T15:25:32Z

🍾

craigcook force-pushed the article branch from 722f362 to b2b9184 Compare May 2, 2018 21:19

craigcook requested a review from alexgibson May 2, 2018 21:20

craigcook added the Dev ready ⚙️ Indicates the story/card/tasks are ready to be coded label May 2, 2018

craigcook mentioned this pull request May 2, 2018

Define general layout stuff #10

Closed

craigcook commented May 2, 2018

View reviewed changes

alexgibson requested changes May 3, 2018

View reviewed changes

craigcook added WIP 🚧 Work In Progress and removed WIP 🚧 Work In Progress labels May 3, 2018

alexgibson reviewed May 4, 2018

View reviewed changes

alexgibson requested changes May 4, 2018

View reviewed changes

craigcook force-pushed the article branch from 07e82db to e429100 Compare May 4, 2018 14:24

craigcook force-pushed the article branch from e429100 to 330bbce Compare May 4, 2018 14:54

alexgibson requested changes May 4, 2018

View reviewed changes

Add article and sidebar menu organisms and main/sidebar template

2d434cc

craigcook force-pushed the article branch from 330bbce to 2d434cc Compare May 4, 2018 15:15

alexgibson approved these changes May 4, 2018

View reviewed changes

alexgibson merged commit d4a2c9d into mozilla:master May 4, 2018

craigcook deleted the article branch May 4, 2018 15:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add article and sidebar menu organisms and main/sidebar template #86

Add article and sidebar menu organisms and main/sidebar template #86

craigcook commented May 2, 2018 •

edited

Loading

craigcook May 2, 2018

alexgibson May 3, 2018 •

edited

Loading

craigcook May 3, 2018

alexgibson left a comment

alexgibson May 3, 2018 •

edited

Loading

alexgibson May 3, 2018

alexgibson May 3, 2018

alexgibson May 3, 2018

alexgibson May 3, 2018

craigcook May 3, 2018

alexgibson May 3, 2018

craigcook May 3, 2018

alexgibson May 3, 2018

alexgibson May 3, 2018

craigcook May 3, 2018

alexgibson May 3, 2018

alexgibson May 3, 2018

alexgibson commented May 4, 2018 •

edited

Loading

alexgibson May 4, 2018

alexgibson commented May 4, 2018 •

edited

Loading

alexgibson left a comment

craigcook commented May 4, 2018

alexgibson commented May 4, 2018 •

edited

Loading

alexgibson left a comment

alexgibson May 4, 2018

craigcook May 4, 2018

alexgibson May 4, 2018

alexgibson May 4, 2018

alexgibson May 4, 2018

alexgibson May 4, 2018 •

edited

Loading

alexgibson left a comment

craigcook commented May 4, 2018

		@@ -0,0 +1,567 @@
		// Define paths
		$font-path: '/assets/protocol/protocol/fonts';

		'../protocol/components/sidebar-menu';


		// Article links... is this a component?

Add article and sidebar menu organisms and main/sidebar template #86

Add article and sidebar menu organisms and main/sidebar template #86

Conversation

craigcook commented May 2, 2018 • edited Loading

Choose a reason for hiding this comment

alexgibson May 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexgibson left a comment

Choose a reason for hiding this comment

alexgibson May 3, 2018 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexgibson commented May 4, 2018 • edited Loading

Choose a reason for hiding this comment

alexgibson commented May 4, 2018 • edited Loading

alexgibson left a comment

Choose a reason for hiding this comment

craigcook commented May 4, 2018

alexgibson commented May 4, 2018 • edited Loading

alexgibson 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

alexgibson May 4, 2018 • edited Loading

Choose a reason for hiding this comment

alexgibson left a comment

Choose a reason for hiding this comment

craigcook commented May 4, 2018

craigcook commented May 2, 2018 •

edited

Loading

alexgibson May 3, 2018 •

edited

Loading

alexgibson May 3, 2018 •

edited

Loading

alexgibson commented May 4, 2018 •

edited

Loading

alexgibson commented May 4, 2018 •

edited

Loading

alexgibson commented May 4, 2018 •

edited

Loading

alexgibson May 4, 2018 •

edited

Loading