feat(dialog): new component #611

mgr34 · 2019-01-11T17:46:11Z

Adds a Dialog Component. golden.json needs to be updated.

note: heading to lunch, plan to add commit later this afternoon with Documentation, but anticipate some changes.

I signed it

…nts#465)

…ranching (material-components#459)

… variant change (material-components#519)

googlebot · 2019-01-11T17:46:25Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

moog16 · 2019-01-11T19:13:38Z

@mgr34 there are files in this PR that shouldn't have been touched, such as top app bar files. It also looks like there are commits from november. Could you please fix this?

mgr34 · 2019-01-11T19:23:11Z

@moog16 uh oh. I really have no idea what happened. Let me see what I can figure out and I'll get back to you.

mgr34 · 2019-01-11T19:30:46Z

@moog16 is my commit history necessary? This task could be more efficient if I would close this PR. create a new branch off of rc0.9.0 and bring in the necessary files and submit a new PR.

moog16

Took an initial look. Some foundation methods aren't being used. Also a lot of typescript errors in test/unit.

Once those are all addressed I'll take another look. Thanks!

package.json

packages/dialog/DialogButton.tsx

packages/dialog/DialogContent.tsx

packages/dialog/package.json

packages/dialog/index.tsx

moog16 · 2019-01-11T19:39:23Z

packages/dialog/index.tsx

+  handleDocumentKeyDown = (e: Event): void =>
+    this.foundation.handleDocumentKeydown(e);
+  // @ts-ignore parameter never used
+  handleLayout = (evt: Event): void => this.foundation.layout();


remove the ts-ignore and evt

packages/dialog/index.tsx

…f empty string

… of PR # 611

packages/dialog/README.md

moog16 · 2019-01-17T18:28:03Z

packages/dialog/index.tsx

@@ -258,7 +260,7 @@ class Dialog<T extends {} = HTMLElement> extends React.Component<
    const container: React.ReactElement<HTMLDivElement> | undefined =
      this.renderContainer(children);
    return (
-      // @ts-ignore
+      // @ts-ignore Tag does not have any construct


is there a github issue associated with this?

Yes, an issue you opened ;)

microsoft/TypeScript#28892

Perhaps I phrased it wrong

sorry - I poorly communicated. Could you please add that issue to the comment on this line

Oh, my bad, I knew what you meant. (changes forthcoming)

test/screenshot/dialog/confirmation.tsx

moog16 · 2019-01-17T18:29:30Z

packages/dialog/DialogButton.tsx

+import Button from '@material/react-button';
+
+
+export interface DialogButtonProps<ButtonProps> extends React.HTMLProps<ButtonProps> {


Sorry I meant

DialogButtonProps<...> extends ButtonProps<...>

moog16 · 2019-01-17T18:30:20Z

packages/dialog/index.tsx

+        aria-modal
+        className={this.classes}
+        id={id}
+        role='alertdialog'


don't forget this one too

moog16 · 2019-01-17T18:30:50Z

packages/dialog/index.tsx

+export type ChildTypes<ChildProps> = DialogTitle<ChildProps> | DialogContent<ChildProps>| DialogFooter<ChildProps>;
+export type ChildProps<T> = DialogTitleProps<T> | DialogContentProps<T> | DialogFooterProps<T>;
+
+export interface DialogProps<T> extends React.HTMLProps<T> {


This one too still needs to be addresed

mgr34

(wait have you not been seeing my previous comments?)

packages/dialog/package.json

packages/dialog/index.tsx

mgr34 · 2019-01-11T22:11:51Z

packages/dialog/DialogContent.tsx

+import {cssClasses} from './constants';
+
+
+export interface DialogContentProps extends React.HTMLProps<HTMLElement>{


Still learning TypeScript, and I guess I am not fully groking it yet.

Its not fully clear to me how to implement the generic. But I looked at other components that allow a user to pass a Tag and I am either seeing it implemented the way I have it. For example, with the Drawer [0] or just having the interface not extend anything. Such as with DrawerTitle [1] or ListItemText [2]

Would the answer here to just change L27 to

export interface DialogContentProps { ... }

[0]

material-components-web-react/packages/drawer/index.tsx

Line 42 in c9bedfe

export interface DrawerProps extends React.HTMLProps<HTMLElement>{

[1] https://github.com/material-components/material-components-web-react/blob/rc0.9.0/packages/drawer/Title.tsx#L4

[2] https://github.com/material-components/material-components-web-react/blob/43cd5796ff70757f7b4ccec668a1a76300aa53fc/packages/list/ListItemText.tsx

mgr34 · 2019-01-11T22:16:14Z

packages/dialog/index.tsx

+
+export type ChildTypes<T> = DialogTitle<T> | DialogContent<T>| DialogFooter<T>;
+
+export interface DialogProps extends React.HTMLProps<HTMLElement> {


See my other comment about generics [0]. But would the proper way to do this be the following? If so does that mean Drawer [1] should also be changed?

export type ChildTypes<T> = DialogTitle<T> | DialogContent<T>| DialogFooter<T>; export interface DialogProps<T> extends React.HTMLProps<T> { autoStackButtons?: boolean; children?: ChildTypes<T>; className?: string; id?: string; onClose?: (action: string) => void; onClosing?: (action: string) => void; onOpen?: () => void; onOpening?: () => void; open?: boolean; tag?: string; }; interface DialogState { classList: Set<string>; } class Dialog<T extends {} = HTMLElement> extends React.Component< DialogProps<T>, DialogState > { .... }

[0] https://github.com/material-components/material-components-web-react/pull/611/files/36deb8cc601396cb29a97023fd3550d9f8200c6b#r247227131

[1] https://github.com/material-components/material-components-web-react/blob/rc0.9.0/packages/drawer/index.tsx

packages/dialog/DialogTitle.tsx

mgr34 · 2019-01-16T21:58:25Z

test/screenshot/top-app-bar/dense.js

@@ -0,0 +1,24 @@
+import React from 'react';


for real, how did any this top-app-bar stuff get in here. ...must investigate.

mgr34 · 2019-01-16T22:14:29Z

packages/dialog/index.tsx

+import {cssClasses, LAYOUT_EVENTS} from './constants';
+import {FocusTrap} from 'focus-trap';
+
+export type ChildTypes<ChildProps> = DialogTitle<ChildProps> | DialogContent<ChildProps>| DialogFooter<ChildProps>;


I think I am understanding what you are saying with this comment, and the one below re: T representing 4 diff types. However, I am struggling to implement this at the moment. I might need to step away for the day and give it a fresh go tomorrow morning.

Ultimately, the reason export type ChildTypes... exists is because I don't want line 44 to extent to long, and more importantly, I need to reuse ChildTypes in renderContainer and renderChild.

mgr34 · 2019-01-17T17:21:36Z

packages/dialog/index.tsx

+import {cssClasses, LAYOUT_EVENTS} from './constants';
+import {FocusTrap} from 'focus-trap';
+
+export type ChildTypes<ChildProps> = DialogTitle<ChildProps> | DialogContent<ChildProps>| DialogFooter<ChildProps>;


Ok, what If I removed the generic and did something like the following?:

export type ChildTypes = ( DialogTitle<DialogTitleProps<{}>> | DialogContent<DialogContentProps<{}>> | DialogFooter<DialogFooterProps<{}>> ); export interface DialogProps<T> extends React.HTMLProps<T> { autoStackButtons?: boolean; children?: ChildTypes | ChildTypes[]; className?: string; escapeKeyAction?: string; id?: string; onClose?: (action: string) => void; onClosing?: (action: string) => void; onOpen?: () => void; onOpening?: () => void; open?: boolean; scrimClickAction?: string; tag?: string; };

the problem I'm seeing in this implementation and the implementation I had prior is I would expect the following to not be valid

<Dialog> <p>should fail because !(DialogTitle | DialogContent | DialogFooter)</p> <DialogContent><p>this is good</p></DialogContent> </Dialog>

But maybe I shouldn't be trying to place such restrictions on the composition pattern. Although, I thought I was enforcing these restrictions. Perhaps this is because Dialog[Title|Content|Footer] have the type signature of type DialogContent<T> = React.ReactElement<T> and allows them to be any ReactElement?--am I using the phrase type signature correctly?

mgr34 · 2019-01-17T18:51:23Z

packages/dialog/index.tsx

@@ -258,7 +260,7 @@ class Dialog<T extends {} = HTMLElement> extends React.Component<
    const container: React.ReactElement<HTMLDivElement> | undefined =
      this.renderContainer(children);
    return (
-      // @ts-ignore
+      // @ts-ignore Tag does not have any construct


Yes, an issue you opened ;)

microsoft/TypeScript#28892

Perhaps I phrased it wrong

test/screenshot/dialog/confirmation.tsx

package.json

moog16 · 2019-01-18T20:23:20Z

packages/dialog/index.tsx

+import {cssClasses, LAYOUT_EVENTS} from './constants';
+import {FocusTrap} from 'focus-trap';
+
+export type ChildTypes = (


This isn't used anywhere...please remove

the export? I do use ChildTypes in test/unit/dialog/index.test.tsx. and in renderContainer, renderChild() and setId.

ah please move to the tests then

(ah just seeing this. I also use this in the Dialog package. Specifically in rednerContainer, renderChild and setId).

test/unit/dialog/index.test.tsx

moog16 · 2019-01-18T20:26:40Z

test/unit/dialog/index.test.tsx

+  wrapper.instance().renderChild =
+    coerceForTesting<(child: ChildTypes, i?: number) => ChildTypes>(td.func());
+  const children: ChildTypes[] =
+    wrapper.instance().props.children as ChildTypes[];


use coerceForTesting() in ./test/unit/helpers/types

that is where I have coerceForTesting imported from. you want me to use it on line 465/466

// change this const children: ChildTypes[] = wrapper.instance().props.children as ChildTypes[];` // to this? const children: ChildTypes[] = coerceForTesting<ChildTypes[]>(wrapper.instance().props.children as ChildTypes[]);

remove the as ChildTypes[]...coerce...() does that for you

moog16

Conditional approval on all other comments...working on screenshot tests.

test/unit/dialog/index.test.tsx

moog16 · 2019-01-18T20:32:42Z

test/unit/dialog/index.test.tsx

+  const wrapper = shallow<Dialog>(<Dialog />);
+  wrapper.instance().foundation.adapter_.addBodyClass('test-class');
+  const body = document.querySelector('body');
+  // @ts-ignore Object is possibly 'null'


moog16 · 2019-01-18T20:38:31Z

test/screenshot/dialog/scrollable.tsx

@@ -0,0 +1,117 @@
+import * as React from 'react';
+import '../../../packages/top-app-bar/index.scss';


Also please remove these from the screenshot tests.

moog16 · 2019-01-18T21:03:16Z

@mgr34

  "dialog/alert": "e0c9b5fd4468c5fff9902b4e8e79c488b0fe8300b08a9620f32ed380b5c2af79",
  "dialog/simple": "2b72930b7711c258d4fead83055ec02b10edebda722901e7ed3ecb7365827dd1",
  "dialog/confirmation": "0d548366130de2aeb85d993a0e9d2030840db0ba10bc49b2721ae05cbd42b058",
  "dialog/scrollable": "91bf3789cc22928df667c96a8a7f1cfdcaad4868c55ab5d830d132249a620f1c"

moog16 · 2019-01-18T21:04:21Z

also need to add

dialogVariants.forEach((variant: string) => {
  urls.push(`dialog/${variant}`);
});

to ./test/screenshot/screenshot-test-urls.tsx

moog16 · 2019-01-22T22:39:06Z

reopened tests #629

moog16 · 2019-01-23T00:14:14Z

#629 passes with the change reflected in 7ed0101.

CLA Signed in first commit. Going to merge.

Matt Goo and others added 23 commits November 30, 2018 14:49

chore(infrastructure): remove all .npmignores (material-components#468)

35b6e47

docs(drawer): fixed closing parens in readme (material-components#460)

c9b88da

docs: update main readme (material-components#472)

69f9333

docs(tab-bar): update readme with more sass imports (material-compone…

8a09dc9

…nts#465)

docs(infrastructure): update release doc to match release candidate b…

bbb9bf2

…ranching (material-components#459)

docs(select): Removed empty row in props documentation (material-comp…

4341029

…onents#483)

docs(list): Minor changes in docs (material-components#484)

e898987

docs(card-media): update readme with aspect ratio requirement (materi…

8165d69

…al-components#504)

docs(tab): Include tab-indicator with Tab documentation. (material-co…

0830172

…mponents#510)

fix(list): minification-safe element type comparison (material-compon…

5e9620e

…ents#509)

docs(checkbox): Use htmlFor in example in Checkbox docs. (material-co…

df112af

…mponents#516)

docs: update broken look to ../screenshot-tests.md (material-componen…

33fb500

…ts#515)

feat(top app bar): dense variant (material-components#520)

4a4e941

feat(top-app-bar): TopAppBarFixedAdjust variants added (material-comp…

160cf3a

…onents#524)

fix(top-app-bar): foundation should be destroyed and reinitialized on…

689b792

… variant change (material-components#519)

feat(dialog): comitting work on dialog

7509d8a

feat(dialog): comitting [unfinished] tests on dialog

e95da5d

feat(dialog): increasing code coverage

aa140a2

feat(dialog): increasing code coverage

1e8d8e6

feat(dialog): increasing code coverage && last commit before TS port

6bb0cac

merging rc0.9.0

cef04db

feat(dialog): Adding a Dialog Component

f7ef506

feat(dialog): bring branch up to date with rc0.9.0

441c6aa

Merge branch 'rc0.9.0' into feat/dialog

0e62e50

Delete yarn.lock

06cb064

moog16 suggested changes Jan 11, 2019

View reviewed changes

mgr34 added 2 commits January 16, 2019 15:36

feat(dialog): still need to set scrimClickAction && escapeKeyAction i…

4776b95

…f empty string

feat(dialog): WIP -- adding some of the changes requested from review…

a76a902

… of PR # 611

moog16 suggested changes Jan 17, 2019

View reviewed changes

mgr34 commented Jan 17, 2019

View reviewed changes

mgr34 added 6 commits January 18, 2019 12:08

feat(dialog): requested changes

56e405b

feat(dialog): DialogButton should extend ButtonProps, but exclude

523341b

feat(dialog): add dialog to scope

178465f

Merge remote-tracking branch 'origin/rc0.9.0' into feat/dialog

10ea0c7

feat(dialog): remove wrong file

9f25f4c

feat(dialog): removing and reverting accidential commits

d1fa9c4

moog16 reviewed Jan 18, 2019

View reviewed changes

package.json Outdated Show resolved Hide resolved

moog16 reviewed Jan 18, 2019

View reviewed changes

test/unit/dialog/index.test.tsx Show resolved Hide resolved

moog16 reviewed Jan 18, 2019

View reviewed changes

moog16 approved these changes Jan 18, 2019

View reviewed changes

moog16 mentioned this pull request Jan 18, 2019

DO NOT MERGE tests: Mgr34 feat/dialog #624

Closed

moog16 reviewed Jan 18, 2019

View reviewed changes

mgr34 added 2 commits January 22, 2019 16:39

feat(dialog): All unit tests now run and other changes

695720f

Merge remote-tracking branch 'origin/rc0.9.0' into feat/dialog

e100fc6

moog16 changed the title ~~Feat/dialog~~ feat(dialog): new component Jan 22, 2019

moog16 mentioned this pull request Jan 22, 2019

DO NOT MERGE Tests: Mgr34 feat/dialog #629

Closed

fix: remove dialog

7ed0101

moog16 merged commit 00c1a42 into material-components:rc0.9.0 Jan 23, 2019

moog16 mentioned this pull request Jan 23, 2019

Add Snackbar component #447

Closed

moog16 pushed a commit that referenced this pull request Jan 23, 2019

feat(dialog): new component (#611)

555e61f

moog16 mentioned this pull request Jan 23, 2019

Add new dialog #239

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(dialog): new component #611

feat(dialog): new component #611

mgr34 commented Jan 11, 2019

googlebot commented Jan 11, 2019

moog16 commented Jan 11, 2019

mgr34 commented Jan 11, 2019

mgr34 commented Jan 11, 2019

moog16 left a comment

moog16 Jan 11, 2019

moog16 Jan 17, 2019

mgr34 Jan 17, 2019

moog16 Jan 17, 2019

mgr34 Jan 17, 2019

moog16 Jan 17, 2019

moog16 Jan 17, 2019

moog16 Jan 17, 2019

mgr34 left a comment

mgr34 Jan 11, 2019

mgr34 Jan 11, 2019

mgr34 Jan 16, 2019

mgr34 Jan 16, 2019

mgr34 Jan 17, 2019

mgr34 Jan 17, 2019

moog16 Jan 18, 2019

mgr34 Jan 18, 2019

moog16 Jan 18, 2019

mgr34 Jan 22, 2019 •

edited

Loading

moog16 Jan 18, 2019

mgr34 Jan 18, 2019

moog16 Jan 18, 2019

moog16 left a comment

moog16 Jan 18, 2019

moog16 Jan 18, 2019

moog16 commented Jan 18, 2019

moog16 commented Jan 18, 2019

moog16 commented Jan 22, 2019

moog16 commented Jan 23, 2019

		import Button from '@material/react-button';


		export interface DialogButtonProps<ButtonProps> extends React.HTMLProps<ButtonProps> {

		import {cssClasses} from './constants';


		export interface DialogContentProps extends React.HTMLProps<HTMLElement>{


		export type ChildTypes<T> = DialogTitle<T> \| DialogContent<T>\| DialogFooter<T>;

		export interface DialogProps extends React.HTMLProps<HTMLElement> {

		@@ -0,0 +1,117 @@
		import * as React from 'react';
		import '../../../packages/top-app-bar/index.scss';

feat(dialog): new component #611

feat(dialog): new component #611

Conversation

mgr34 commented Jan 11, 2019

googlebot commented Jan 11, 2019

moog16 commented Jan 11, 2019

mgr34 commented Jan 11, 2019

mgr34 commented Jan 11, 2019

moog16 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

mgr34 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

mgr34 Jan 22, 2019 • 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

moog16 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moog16 commented Jan 18, 2019

moog16 commented Jan 18, 2019

moog16 commented Jan 22, 2019

moog16 commented Jan 23, 2019

mgr34 Jan 22, 2019 •

edited

Loading