Add modal organism (Fixes #78) #118

alexgibson · 2018-05-18T14:27:27Z

Adds modal organism (replicating style seen on the new mozilla.org home page)
Adds a light-links mixin, for better link contrast on dark backgrounds.

Note: I had originally hoped to use <dialog> and a polyfill, but I discovered some a11y shortcomings in the polyfill that I don't think we can lose to justify switching yet 😢. Maybe once more browsers have native support. For now, this is a direct port of the bedrock modal JS (just replacing the jQuery for native JS).

~~We still need to bikeshed the styles here (including things like the close icon, which needs to be moved to the design assets repo once we decide what to use).~~

Demo: https://demo1--mozilla-protocol.netlify.com/patterns/organisms/modal.html

alexgibson · 2018-05-18T14:30:11Z

src/assets/img/icons/ui/close-lg.svg

@@ -0,0 +1 @@
+<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 42 42"><style>.st0{fill:none;stroke:#fff;stroke-miterlimit:10}</style><path class="st0" d="M0 0l42 42M0 42L42 0"/></svg>


Once we decide on an icon to use, this needs to be moved to the design assets repo.

alexgibson · 2018-05-18T14:33:49Z

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

+                a:hover,
+                a:focus,
+                a:active {
+                    color: darken($color-secondary, 5%);


Should this be a new link style atom?

alexgibson · 2018-05-18T14:49:47Z

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

+    a:hover,
+    a:focus,
+    a:active {
+        color: darken($color-secondary, 5%);


Should this link style be a new atom?

I've been thinking we probably need to standardize a style for links on dark backgrounds and make a mixin for it, something like @include light-links;

Added a light-links mixin 👍

vincejoyiv · 2018-05-22T15:57:12Z

@alexgibson I'm going to review this today. My thinking is that it can be improved, and become a better foundation for a standard modal component—it's a bit weird to start with a specialized component like a video player and then try to reverse engineer a simple modal from that, but let's give it a try.

alexgibson · 2018-05-23T08:45:13Z

Ok, looks like we already have a close icon in the design-assets repo, so I updated the modal here to reuse that instead of introduce a new icon. It also has slightly thicker paths, so contrast is improved.

craigcook

Looks good! We should probably use our new naming convention for the noscroll class but that's my only real nitpick.

There are some suggestions for more notes/documentation but those can wait and needn't block merging. The close button could use a better :focus state for keyboards but I know there will be some style changes forthcoming and we can improve it then.

craigcook · 2018-05-23T15:28:14Z

src/patterns/organisms/modal/modal.hbs

+          title: 'Example headline with 35 characters',
+          closeText: 'Close modal',
+          onCreate: function() {
+              console.log('Modal opened');


Maybe worth adding a comment (directly in the script or just in the notes) that this is a demonstration, we don't want people including these console bits in production.

On second thought the notes already explain what onCreate and onDestroy do and this whole thing is clearly just an example already, maybe no need to drive that point home any further.

craigcook · 2018-05-23T15:31:46Z

src/patterns/organisms/modal/modal.hbs

+
+        Mozilla.Modal.createModal(e.target, content, {
+          title: 'Example headline with 35 characters',
+          closeText: 'Close modal',


We probably need some better documentation around this, but we can at least comment that this is a demo and ideally one shouldn't have translatable strings in JS (unless you have a system for translating JS files or aren't localizing anyway). The title could be a heading in the DOM, closeText could be a data attribute, etc.

I think it may be best (for now at least) to try and shy away from too much hand-holding when it comes to l10n, as different sites all have different methods of doing things. It's not directly related to the component itself, so to avoid over-complicating the instructions here, I think a generic l10n section may be a better time to explain this type of thing?

Yeah, I'm probably overly paranoid about people following this example when hopefully it's clear that it's JUST an example. We're working on general l10n guidelines and "no strings in JS" is a good rule.

craigcook · 2018-05-23T15:36:02Z

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

+@import '../includes/lib';
+@import '../includes/animations';
+
+html.noscroll {


mzp-is-noscroll?

craigcook · 2018-05-23T15:36:21Z

src/assets/sass/protocol/includes/_animations.scss

+    }
+}
+
+@keyframes mzp-a-fade-in {


We should add the a- prefix to our naming convention.

craigcook · 2018-05-23T15:37:33Z

src/assets/js/protocol/protocol-modal.js

+            '</div>';
+
+        if ((options && !options.allowScroll) || isSmallViewport) {
+            html.classList.add('noscroll');


Should this class follow the naming convention for states? So it would be mzp-is-noscroll

alexgibson · 2018-05-23T17:38:02Z

Thanks @craigcook - I updated the noscroll class, and also added a basic focus outline for the close button (better than the miniscule scale effect).

craigcook

🛵

alexgibson added WIP 🚧 Work In Progress Do Not Merge ⚠️ Do Not Merge labels May 18, 2018

alexgibson commented May 18, 2018

View reviewed changes

alexgibson requested a review from vincejoyiv May 18, 2018 14:31

alexgibson commented May 18, 2018

View reviewed changes

alexgibson removed the WIP 🚧 Work In Progress label May 21, 2018

craigcook self-assigned this May 21, 2018

alexgibson removed the Do Not Merge ⚠️ Do Not Merge label May 23, 2018

craigcook requested changes May 23, 2018

View reviewed changes

Add modal organism (Fixes #78)

e577315

craigcook approved these changes May 23, 2018

View reviewed changes

craigcook merged commit 890d80e into mozilla:master May 23, 2018

alexgibson deleted the port-bedrock-modal branch May 25, 2018 09:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add modal organism (Fixes #78) #118

Add modal organism (Fixes #78) #118

alexgibson commented May 18, 2018 •

edited

Loading

alexgibson May 18, 2018 •

edited

Loading

alexgibson May 18, 2018

alexgibson May 18, 2018

craigcook May 18, 2018

alexgibson May 21, 2018

vincejoyiv commented May 22, 2018

alexgibson commented May 23, 2018

craigcook left a comment

craigcook May 23, 2018

craigcook May 23, 2018

craigcook May 23, 2018

alexgibson May 23, 2018

craigcook May 23, 2018

craigcook May 23, 2018

craigcook May 23, 2018

craigcook May 23, 2018

alexgibson commented May 23, 2018

craigcook left a comment

		@@ -0,0 +1 @@
		<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 42 42"><style>.st0{fill:none;stroke:#fff;stroke-miterlimit:10}</style><path class="st0" d="M0 0l42 42M0 42L42 0"/></svg>

Add modal organism (Fixes #78) #118

Add modal organism (Fixes #78) #118

Conversation

alexgibson commented May 18, 2018 • edited Loading

alexgibson May 18, 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

vincejoyiv commented May 22, 2018

alexgibson commented May 23, 2018

craigcook 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

alexgibson commented May 23, 2018

craigcook left a comment

Choose a reason for hiding this comment

alexgibson commented May 18, 2018 •

edited

Loading

alexgibson May 18, 2018 •

edited

Loading