SlidePanel #18

bitpshr · 2017-02-03T19:29:12Z

Type: feature

The following has been addressed in the PR:

There is a related issue
All code matches the style guide
Unit or Functional tests are included in the PR

Description:

This PR introduces a SlidePanel component, updates the existing Dialog component to the new widget API, and adds custom element wrappers for each. It also adds accessibility features to the Dialog based on suggestions from @smhigley. Focus accessibility was omitted until we work through a focus manager.

Initial SlidePanel API:

align?: string;
open?: boolean;
underlay?: boolean;
width?: number;
onOpen?(): void;
onRequestClose?(): void;

Resolves #2, Resolves #12, Resolves #17

codecov-io · 2017-02-04T13:07:57Z

Codecov Report

Merging #18 into master will not impact coverage.

@@          Coverage Diff          @@
##           master    #18   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           2      3    +1     
  Lines          26    105   +79     
  Branches       13     44   +31     
=====================================
+ Hits           26    105   +79

Impacted Files	Coverage Δ
src/components/dialog/createDialog.ts	`100% <100%> (ø)`	✅
src/components/slidePanel/createSlidePanel.ts	`100% <100%> (ø)`
src/main.ts	`100% <100%> (ø)`	✅

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 0692f44...ca67210. Read the comment docs.

agubler · 2017-02-04T17:27:45Z

src/components/slidePanel/createSlidePanel.ts

+
+export interface SlidePanelFactory extends WidgetFactory<SlidePanel, SlidePanelProperties> { };
+
+let content: HTMLElement;


Do these not have to be stored in a weakmap keyed by the instance? Otherwise having multiple instances will mess with the values.

You're right. Do you have an example of how that would look?

Something like this I'd imagine.

interface InternalState { content: any; } const internalStateMap = new Weakmap<SlidePanel, InternalState>(); // and something like this in the initialise initialize(instance) { internalStateMap.set(instance, initialValues): }

agubler · 2017-02-04T17:29:57Z

Can we add a custom element wrapper by default for all our components? There are instructions on the readme.

agubler

A few comments/questions

agubler · 2017-02-06T10:04:06Z

package.json

    "@dojo/compose": "2.0.0-beta.21",
    "@dojo/core": "2.0.0-alpha.20",
    "@dojo/has": "2.0.0-alpha.7",
    "@dojo/i18n": "2.0.0-alpha.5",
    "@dojo/shim": "2.0.0-beta.8",
+    "@dojo/widget-core": "^2.0.0-alpha.23",


we are trying to work with pinned dependencies at the moment.

agubler · 2017-02-06T10:05:49Z

src/components/slidePanel/createSlidePanel.ts

+import * as animations from '../../styles/animations.css';
+import themeable, { ThemeableMixin } from '@dojo/widget-core/mixins/themeable';
+
+export interface SlidePanelProperties extends WidgetProperties {


let's add some jsdoc for the properties (and other exported APIs)

agubler · 2017-02-06T10:06:39Z

src/components/slidePanel/createSlidePanel.ts

+			} = this.properties;
+
+			// Current pointer position
+			let currentX = event.type === 'touchmove' ? event.changedTouches[0].screenX : event.pageX;


presume these can both be const?

agubler · 2017-02-06T10:07:27Z

src/components/slidePanel/createSlidePanel.ts

+				onRequestClose && onRequestClose();
+			}
+			// If the underlay was clicked
+			else if (delta > -5 && delta < 5 && (<HTMLElement> event.target).classList.contains(css.underlay)) {


I think we might have a type somewhere for this (I'll see if I can find it)

agubler · 2017-02-06T10:18:24Z

src/components/slidePanel/createSlidePanel.ts

+			const classes: {[key: string]: any} = {};
+			const styles: {[key: string]: any} = {};
+
+			classes[css.content] = true;


I think that we should be using the this.class( function as it effectively manages the creation of any required false classes, so for this example you could just have an array of classNames that you want to be set and then spread them this.classes(...classesToSet).get()?

const classes = [css.content]; if (open && !state.wasOpen) { classes.push(css.slideIn); } else if (!open && state.wasOpen) { classes.push(css.slideOut); if (state.transform !== 0) { styles['transform'] = `translateX(${ align === 'left' ? '-' : '' }${ state.transform }%)`; } } const content = v('div', { key: 'content', classes: this.classes(...classes).get(), styles, afterCreate: this.afterCreate }, this.children);

bitpshr · 2017-02-07T02:17:38Z

@agubler: Ready for another review when you get a chance. Want to make sure my JSDoc looks ok, I was a little confused what to document for returned factory functions.

agubler

couple small questions?

agubler · 2017-02-07T15:54:57Z

src/components/dialog/createDialog.ts

+ * @property	{Function?}		onCloseClick		Event handler for when the close button is clicked
+ * @property	{Function?}		onUnderlayClick		Event handler for when a click occurs outside the dialog
+ */
+export type Dialog = Widget<DialogProperties> & ThemeableMixin & {


these are mandatory really? as you implement them as the author?

agubler · 2017-02-07T15:55:59Z

src/components/slidePanel/createSlidePanel.ts

+ * @property	{Function?}		afterCreate			Called after the panel is rendered as DOM
+ * @property	{Function?}		onTransitionEnd		Event handler for when the panel finishes any animation
+ */
+export type SlidePanel = Widget<SlidePanelProperties> & ThemeableMixin & {


pretty much same comment, they are all implemented below so can remove the ??

tomdye · 2017-02-06T10:51:34Z

src/components/slidePanel/createSlidePanel.ts

+};
+
+export type SlidePanel = Widget<SlidePanelProperties> & ThemeableMixin & {
+	onSwipeStart?(event: MouseEvent & TouchEvent): void;


Should these be optional? They're all implemented and part of the widget so it feels like they should not.

bitpshr · 2017-02-07T18:24:06Z

Ready for another look.

rorticus · 2017-02-07T18:44:43Z

src/components/dialog/createDialogElement.ts

+			},
+			{
+				attributeName: 'modal',
+				value: value => Boolean(value)


These Boolean conversions aren't working quite right. Boolean('false') and Boolean('0') return true.

rorticus

The custom element wrappers look good 👍 I created a little test page with a slide panel and was able to interact with it as a custom element.

bitpshr · 2017-02-07T19:42:39Z

Party on.

bitpshr added 6 commits February 1, 2017 17:25

Initial SlidePanel work

d7ada0d

Add click cushion

2431bf6

Update SlidePanel and Dialog

a336435

grunt-dojo2 should be a dev dep

8b88e0c

Remove log

a2af2c7

Allow width to be specified as property

e8ed968

bitpshr added 5 commits February 4, 2017 09:19

Increase coverage

aff3b1a

Increase coverage more

0a0a61e

100% coverage

3ccca79

Use const where possible

ec8b59f

Add comments

66af5f0

agubler reviewed Feb 4, 2017

View reviewed changes

More comments

a01c2aa

bitpshr added 4 commits February 4, 2017 13:14

Use WeakMap to manage instance-specific state

bb17e98

Add custom element wrappers for Dialog and SlidePanel

7d3b0b0

Exclude custom element wrappers from tests

8803e6f

Use WeakMap shim for IE10

764d84b

agubler requested changes Feb 6, 2017

View reviewed changes

tomdye mentioned this pull request Feb 6, 2017

Update Dialog to use latest themeable mixin #15

Closed

3 tasks

dylans added this to the 2017.02 milestone Feb 6, 2017

bitpshr added 2 commits February 6, 2017 16:35

PR feedback

41d509f

JSDoc

abfb987

bitpshr added 2 commits February 7, 2017 10:10

Dialog accessability

005f218

typo

af56e8c

agubler reviewed Feb 7, 2017

View reviewed changes

tomdye requested changes Feb 7, 2017

View reviewed changes

Event handlers are not optional

022d2db

Add new properties to custom element wrapper

c0330a8

rorticus reviewed Feb 7, 2017

View reviewed changes

Update boolean conversions on custom element wrappers

ca67210

rorticus approved these changes Feb 7, 2017

View reviewed changes

agubler approved these changes Feb 7, 2017

View reviewed changes

bitpshr merged commit 6c8a5d1 into dojo:master Feb 7, 2017

bitpshr deleted the slidepanel branch February 7, 2017 19:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SlidePanel #18

SlidePanel #18

bitpshr commented Feb 3, 2017 •

edited

Loading

codecov-io commented Feb 4, 2017 •

edited

Loading

agubler Feb 4, 2017

bitpshr Feb 4, 2017

agubler Feb 4, 2017

agubler commented Feb 4, 2017 •

edited

Loading

agubler left a comment

agubler Feb 6, 2017

agubler Feb 6, 2017

agubler Feb 6, 2017

agubler Feb 6, 2017

agubler Feb 6, 2017

bitpshr commented Feb 7, 2017

agubler left a comment

agubler Feb 7, 2017

bitpshr Feb 7, 2017

agubler Feb 7, 2017

bitpshr Feb 7, 2017

tomdye Feb 6, 2017

bitpshr Feb 7, 2017

bitpshr commented Feb 7, 2017

rorticus Feb 7, 2017

rorticus left a comment

bitpshr commented Feb 7, 2017


		export interface SlidePanelFactory extends WidgetFactory<SlidePanel, SlidePanelProperties> { };

		let content: HTMLElement;

SlidePanel #18

SlidePanel #18

Conversation

bitpshr commented Feb 3, 2017 • edited Loading

codecov-io commented Feb 4, 2017 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agubler commented Feb 4, 2017 • edited Loading

agubler 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

bitpshr commented Feb 7, 2017

agubler 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

bitpshr commented Feb 7, 2017

Choose a reason for hiding this comment

rorticus left a comment

Choose a reason for hiding this comment

bitpshr commented Feb 7, 2017

bitpshr commented Feb 3, 2017 •

edited

Loading

codecov-io commented Feb 4, 2017 •

edited

Loading

agubler commented Feb 4, 2017 •

edited

Loading