feat(drawer): Implement persistent drawer #488

lynnmercier · 2017-04-06T19:26:38Z

Resolves #15, Implement persistent drawer by re-using the "slidable" logic from temporary drawer.

…ry.foundation.test.js

codecov-io · 2017-04-06T19:37:05Z

Codecov Report

Merging #488 into master will decrease coverage by 0.03%.
The diff coverage is 97.9%.

@@            Coverage Diff             @@
##           master     #488      +/-   ##
==========================================
- Coverage   99.05%   99.02%   -0.04%     
==========================================
  Files          44       45       +1     
  Lines        2016     2050      +34     
  Branches      249      251       +2     
==========================================
+ Hits         1997     2030      +33     
- Misses         19       20       +1

Impacted Files	Coverage Δ
packages/mdc-drawer/temporary/constants.js	`100% <ø> (ø)`	⬆️
packages/mdc-drawer/temporary/index.js	`97.29% <ø> (ø)`	⬆️
packages/mdc-drawer/slidable/constants.js	`100% <100%> (ø)`	⬆️
packages/mdc-drawer/persistent/constants.js	`100% <100%> (ø)`	⬆️
packages/mdc-drawer/persistent/foundation.js	`100% <100%> (ø)`	⬆️
packages/mdc-drawer/temporary/foundation.js	`100% <100%> (ø)`	⬆️
packages/mdc-drawer/persistent/index.js	`97.05% <97.05%> (ø)`
packages/mdc-drawer/slidable/foundation.js	`97.69% <97.69%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update daf17d5...e8b723e. Read the comment docs.

traviskaufman · 2017-04-07T15:27:09Z

Looks like the coverage diff is failing. Once that's fixed I'll review!

traviskaufman

Really excellent work here! And thank you especially for taking the time to refactor out the common logic between temp and persistent drawers. Besides the inline comments, a few general comments:

I'd add a very simple README inside of the slidable dir denoting its purpose and intent to be used only as a base class for our own drawers, not for public API use. This will make it clear to contributors and users what its purpose is without digging through our code.
It doesn't look like the main README has been updated with any usage docs around persistent drawers. This info needs to be added to the README, along with the adapter API table which is required for all components. One note here is that the adapter API should outline all methods needed for the SlidableDrawer adapter as well, as if it were just one object. This denotes how using a slidable drawer base class is purely an implementation detail on our end, and has no effect on how adapters are shaped and passed to the foundation from a client code POV.

traviskaufman · 2017-04-10T19:05:15Z

packages/mdc-drawer/persistent/constants.js

+
+import {focusableElements} from '../slidable';
+
+const ROOT = 'mdc-persistent-drawer';


I know we have this littered throughout our codebase already, but we actually need to ensure moving forward that these are all static strings so that they interop with closure stylesheet's renaming of css classes / strings the correct way.

export const cssClasses = { ROOT: 'mdc-persistent-drawer', OPEN: 'mdc-persistent-drawer--open', ANIMATING: 'mdc-persistent-drawer--animating', }; export const strings = { DRAWER_SELECTOR: '.mdc-persistent-drawer__drawer', FOCUSABLE_ELEMENTS: focusableElements, };

Our bad 😐

traviskaufman · 2017-04-10T19:51:35Z

packages/mdc-drawer/persistent/foundation.js

+  }
+
+  static get defaultAdapter() {
+    const defaultAdapter = MDCSlidableDrawerFoundation.defaultAdapter;


nit: You could simplify this by using Object.assign

return Object.assign(MDCSlidableDrawerFoundation.defaultAdapter, { isRoot: () => false, });

traviskaufman · 2017-04-10T19:55:02Z

packages/mdc-drawer/persistent/foundation.js

+      MDCPersistentDrawerFoundation.cssClasses.OPEN);
+  }
+
+  isRootTransitioningElement_(el) {


Per our authoring components guide, we try very hard not to make references to _element_s or anything that makes assumptions about a host environment in our foundation / adapter code.

I imagine that this method gets called likely in response to an event, to which the event target is passed to this method. In that case, we usually tend to use the term eventTarget in place of element. E.g.

material-components-web/packages/mdc-menu/simple/foundation.js

Line 54 in 4d0d587

getIndexForEventTarget: (/* target: EventTarget */) => /* number */ 0,

traviskaufman · 2017-04-10T19:57:23Z

packages/mdc-drawer/persistent/index.js

+import * as util from '../util';
+
+export {MDCPersistentDrawerFoundation};
+


we may want to consider exporting util here. Users have asked for this in the past with other utility modules we've made for components.

I went ahead and exported util for temporary/index.js too

traviskaufman · 2017-04-10T20:01:40Z

packages/mdc-drawer/persistent/mdc-persistent-drawer.scss

+  }
+
+  &--animating {
+    transition: mdc-animation-enter(width, $mdc-slidable-drawer-transition-time);


Hmm, you definitely don't want to be animating width here if at all possible, as it has severe performance implications, especially considering that this will tend to have implications on how the entire page is laid out. I'd be okay with a jump-cut here. Otherwise, I'd look into using something like the FLIP model to see if we can achieve the same affect using transforms.

Going with jump-cut for now...but I left the extra div element (aka .mdc-persistent-drawer and .mdc-persistent-drawer__drawer) since I figured clients will want to animate something like width?

traviskaufman · 2017-04-10T20:04:04Z

packages/mdc-drawer/persistent/mdc-persistent-drawer.scss

+    @include mdc-rtl-reflexive-position(left, 0);
+
+    @include mdc-theme-dark {
+      background: #212121;


Not sure we want to hard-code BG colors here, as this gives the user a lot less thematic control and assumes that this is the color of the background we want to use for dark themes. I'd take an approach similar to dialog here (source).

I went ahead and refactored permanent drawers to "not hard-code BG colors" too

traviskaufman · 2017-04-10T20:05:55Z

packages/mdc-drawer/index.js

@@ -15,5 +15,7 @@
 */

 import * as util from './util';
+export {MDCSlidableDrawerFoundation} from './slidable';


Not sure if we want to export this particular foundation here since it's meant to be subclassed by our drawer implementations, and we want to reduce the API surface such that we're not encouraging users to make their own versions of Material Design drawers.

traviskaufman · 2017-04-10T20:08:42Z

packages/mdc-drawer/slidable/constants.js

+ * limitations under the License.
+ */
+
+export const focusableElements =


nit: This should be named FOCUSABLE_ELEMENTS to adhere to constants conventions.

traviskaufman · 2017-04-10T20:14:28Z

packages/mdc-drawer/slidable/_mixins.css

+ * limitations under the License.
+ */
+
+@import "@material/rtl/mixins";


nit: looks like there are a few magic numbers (107%, 20px) here. Perhaps they should be refactored into scss vars?

Honestly...I have no idea, they were existing magic numbers in temporary drawers.

I've isolated them to slideable's _mixins.scss in this Pull Request. I think a follow up pull request might want to investigate these magic numbers more

didn't realize they came from temporary drawer. SGTM for addressing in a different PR.

traviskaufman · 2017-04-10T20:19:28Z

packages/mdc-drawer/temporary/foundation.js

-      getDrawerWidth: () => /* number */ 0,
-      isDrawer: (/* el: Element */) => /* boolean */ false,
-    };
+    const defaultAdapter = MDCSlidableDrawerFoundation.defaultAdapter;


Same comment as in persistent drawer regarding Object.assign.

lynnmercier · 2017-04-10T22:59:07Z

Updated based on comments, PTAL

traviskaufman

Mostly just nits / small changes this time around!

traviskaufman · 2017-04-11T23:23:27Z

packages/mdc-drawer/README.md

@@ -70,6 +70,131 @@ CSS classes:
 | `mdc-permanent-drawer__content`        | Mandatory. Needs to be set on the container node for the drawer content.   |
 | `mdc-permanent-drawer__toolbar-spacer` | Optional. Add to node to provide the matching amount of space for toolbar. |

+## Persistent drawer usage
+
+A persistent drawers can toggle open or closed. The drawer sits on the same surface elevation as the content. It is closed by default. When the drawer is outside of the page grid and opens, the drawer forces other content to change size and adapt to the smaller viewport. Persistent drawers stay open until closed by the user.


Maybe change this first sentence to "Persistent drawers can be toggled open or closed"?

traviskaufman · 2017-04-11T23:24:57Z

packages/mdc-drawer/persistent/constants.js

+
+export const strings = {
+  DRAWER_SELECTOR: '.mdc-persistent-drawer__drawer',
+  FOCUSABLE_ELEMENTS: FOCUSABLE_ELEMENTS,


nit: you can just the object shorthand property here

export const strings = { DRAWER_SELECTOR: '.mdc-persistent-drawer__drawer', FOCUSABLE_ELEMENTS, };

traviskaufman · 2017-04-11T23:26:35Z

packages/mdc-drawer/persistent/mdc-persistent-drawer.scss

+    @include mdc-rtl-reflexive-position(left, 0);
+
+    @include mdc-theme-dark {
+    background-color: $mdc-persistent-drawer-dark-theme-bg-color;


supernit: seems like the indentation is off here. V surprised stylelint didn't catch that.

traviskaufman · 2017-04-11T23:27:37Z

packages/mdc-drawer/slidable/_mixins.css

+ * limitations under the License.
+ */
+
+@import "@material/rtl/mixins";


didn't realize they came from temporary drawer. SGTM for addressing in a different PR.

traviskaufman · 2017-04-11T23:29:07Z

packages/mdc-drawer/slidable/foundation.js

+    this.openCssClass_ = openCssClass;
+
+    this.transitionEndHandler_ = (ev) => {
+      if (this.isRootTransitioningElement_(ev.target)) {


should this be isRootTransitioningEventTarget_ ?

traviskaufman · 2017-04-11T23:31:38Z

test/unit/mdc-drawer/mdc-persistent-drawer.js

@@ -0,0 +1,284 @@
+/**


This should be named mdc-persistent-drawer.test.js. It actually might not be getting run otherwise...

Also FYI chai has assert.{isTrue,isFalse} which is a strict boolean test rather than just a test for truthiness / falsiness.

oops, totally was not running...well done lynn...

traviskaufman · 2017-04-11T23:35:13Z

test/unit/mdc-drawer/mdc-persistent-drawer.js

+  `;
+  document.body.appendChild(root);
+  const component = new MDCPersistentDrawer(root);
+  assert.isOk(component.getDefaultFoundation().adapter_.isRtl());


you'll want to make sure to remove the root element from document.body before the test ends, here and below.

traviskaufman · 2017-04-11T23:37:21Z

test/unit/mdc-drawer/persistent.foundation.test.js

+});
+
+test('defaultAdapter returns a complete adapter implementation', () => {
+  const {defaultAdapter} = MDCPersistentDrawerFoundation;


there's a verifyDefaultAdapter helper method in test/helpers/foundation which you can use for this

traviskaufman · 2017-04-11T23:37:58Z

test/unit/mdc-drawer/slidable.foundation.test.js

+suite('MDCSlidableDrawerFoundation');
+
+test('defaultAdapter returns a complete adapter implementation', () => {
+  const {defaultAdapter} = MDCSlidableDrawerFoundation;


same comment as above re verifyDefaultAdapter

traviskaufman · 2017-04-11T23:46:09Z

demos/drawer/persistent-drawer.html

+      </nav>
+    </aside>
+    <div class="demo-content">
+      <header class="mdc-toolbar mdc-elevation--z4">


I think this DOM needs to be updated to reflect recent toolbar changes:

<header class="mdc-toolbar mdc-elevation--z4"> <div class="mdc-toolbar__row"> <section class="mdc-toolbar__section mdc-toolbar__section--align-start"> <button class="demo-menu material-icons">menu</button> </section> </div> </header>

…e 'drawer' element as the root transitioning element, now that I removed the transition property from the component's root element

lynnmercier · 2017-04-12T16:50:15Z

Updated based on comments, PTAL

traviskaufman

💎 💯 🔥 🚀 One nit but besides that LGTM! Awesome stuff here, especially around the refactoring.

traviskaufman · 2017-04-12T18:01:37Z

packages/mdc-drawer/README.md

+| `restoreElementTabState(el: Element) => void` | Restores the saved tab index (if any) for an element. |
+| `makeElementUntabbable(el: Element) => void` | Makes an element untabbable. |
+| `isRtl() => boolean` | Returns boolean indicating whether the current environment is RTL. |
+| `isRoot(el: Element) => boolean` | Returns boolean indicating whether the provided element is the root container element. |


Doesn't look like this method is used anywhere anymore. Left over from a previous iteration?

hmm nope. fixed

lynnmercier added 12 commits April 5, 2017 14:08

adding persistent drawer with demo

28fecc7

adding moveable drawer, and refactor persistent to use it

aab45d5

update temporary drawer to use movable drawer

931eb56

missed one

7acff9f

fixing tests by fixing my lint errors, and one tiny change to tempora…

cab571a

…ry.foundation.test.js

fix method names

3078a37

rename movable to slidable

290ac33

refactor slidable drawer to take css classes as input

e32fb41

lint fixes

d2989b2

refactor most of temporary drawer tests into slidable drawer tests

669f7d1

refactor css to share slidable drawer mixins/variables

af45122

fix width issue on slideable drawer

c817fb6

lynnmercier requested a review from traviskaufman April 6, 2017 19:26

Merge branch 'master' into feat/permanent-drawer

f07c59a

lynnmercier added 2 commits April 10, 2017 10:11

Merge branch 'master' into feat/permanent-drawer

5ae0715

add more tests to persistent and temporary drawers

99ebf9e

traviskaufman suggested changes Apr 10, 2017

View reviewed changes

add/update READMEs, address comments in PR, and add one more test class

e67aa08

Merge branch 'master' into feat/permanent-drawer

84913bf

traviskaufman suggested changes Apr 11, 2017

View reviewed changes

lynnmercier added 2 commits April 12, 2017 09:19

Merge branch 'master' into feat/permanent-drawer

b16d947

updated based on comments, and changed persistent drawer to treate th…

7aae73e

…e 'drawer' element as the root transitioning element, now that I removed the transition property from the component's root element

traviskaufman approved these changes Apr 12, 2017

View reviewed changes

fix readme

e8b723e

lynnmercier merged commit 79a2352 into master Apr 12, 2017

lynnmercier deleted the feat/permanent-drawer branch April 12, 2017 18:11


		import {focusableElements} from '../slidable';

		const ROOT = 'mdc-persistent-drawer';

		import * as util from '../util';

		export {MDCPersistentDrawerFoundation};

feat(drawer): Implement persistent drawer #488

feat(drawer): Implement persistent drawer #488

Conversation

lynnmercier commented Apr 6, 2017

codecov-io commented Apr 6, 2017 • edited Loading

Codecov Report

traviskaufman commented Apr 7, 2017

traviskaufman 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

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

lynnmercier commented Apr 10, 2017

traviskaufman 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lynnmercier commented Apr 12, 2017

traviskaufman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Apr 6, 2017 •

edited

Loading