feat(example): add logout confirmation by koumatsumoto · Pull Request #1287 · ngrx/platform

koumatsumoto · 2018-08-24T15:59:41Z

PR Checklist

Please check if your PR fulfills the following requirements:

The commit message follows our guidelines: https://github.com/ngrx/platform/blob/master/CONTRIBUTING.md#commit
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[x] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] Other... Please describe:

What is the current behavior?

Issue Number: #1271

What is the new behavior?

Show sample code for handling a confirmation prompt using an Actions and Effects with an Angular Material dialog.

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

Closes #1271

coveralls · 2018-08-24T16:02:49Z

Coverage remained the same at 88.755% when pulling 3ab14fc on kouMatsumoto:add-logout-confirmation into 466e2cd on ngrx:master.

timdeschryver · 2018-08-25T10:17:23Z

+    expect(fixture).toMatchSnapshot();
+  });
+
+  it('should have buttons that are applied [mat-dialog-close]', () => {


I think this test isn't needed, because we already have a snapshot above.

thankyou! I'll remove it.

timdeschryver · 2018-08-25T10:17:38Z

+
+describe('Logout Confirmation Dialog', () => {
+  let fixture: ComponentFixture<LogoutConfirmationDialogComponent>;
+  let instance: LogoutConfirmationDialogComponent;


Can be removed

timdeschryver · 2018-08-25T10:19:29Z

+
+      return dialogRef.afterClosed();
+    }),
+    filter(result => result === 'OK'),


Use LogoutConfirmationDialogResult.OK instead of the string 'OK'

Thankyou! I'll use string enum instead.

timdeschryver · 2018-08-25T10:20:10Z

+  logoutConfirmation$ = this.actions$.pipe(
+    ofType(AuthActionTypes.LogoutConfirmation),
+    exhaustMap(() => {
+      const dialogRef = this.dialog.open<


I think the types aren't needed?

const dialogRef = this.dialog.open(LogoutConfirmationDialogComponent)

I think dialog can not infer result value.
Should we omit it?

You're right, we can leave it as it is.

Change LogoutConfirmatinoDialogResultTypes to boolean. See comment above in dialog component.

koumatsumoto · 2018-08-27T04:48:59Z

@timdeschryver
I updated changes with your advices.
Please review me, thanks!

brandonroberts · 2018-08-27T12:42:45Z

+  `,
+})
+export class LogoutConfirmationDialogComponent {
+  successValue = LogoutConfirmationDialogResults.OK;


Instead of an enum, just map OK to true and Cancel to false. The dialog will also dismiss and return false by clicking outside its container.

brandonroberts · 2018-08-27T12:46:49Z

+  logoutConfirmation$ = this.actions$.pipe(
+    ofType(AuthActionTypes.LogoutConfirmation),
+    exhaustMap(() => {
+      const dialogRef = this.dialog.open<


Change LogoutConfirmatinoDialogResultTypes to boolean. See comment above in dialog component.

koumatsumoto · 2018-08-27T13:21:14Z

@brandonroberts
Thank you for the review!
I updated my changes, please see it.

brandonroberts · 2018-08-27T17:35:04Z

@koumatsumoto will you rebase this on master again? Thanks

brandonroberts · 2018-08-27T23:53:59Z

+@Component({
+  template: `
+    <h2 mat-dialog-title>Logout</h2>
+    <mat-dialog-content>Do you really want to logout?</mat-dialog-content>


Change to Are you sure you want to logout?

brandonroberts · 2018-08-27T23:55:12Z

+      <button mat-button [mat-dialog-close]="false">Cancel</button>
+      <button mat-button [mat-dialog-close]="true">OK</button>
+    </mat-dialog-actions>
+  `,


Add some styles to the component

:host { display: block; width: 100%; max-width: 300px; } mat-dialog-actions { display: flex; justify-content: flex-end; } [mat-button] { padding: 0; }

brandonroberts · 2018-08-27T23:57:54Z


  beforeEach(() => {
    TestBed.configureTestingModule({
+      imports: [MatButtonModule, MatDialogModule],


These imports aren't needed for the effects

refactor(example): add a line-break refactor(example): fix incorrect value

…o dialog result

koumatsumoto · 2018-08-28T01:02:54Z

@brandonroberts
I updated changes, please continue your review 😄

koumatsumoto force-pushed the add-logout-confirmation branch 2 times, most recently from 0f6713b to 86f4be8 Compare August 24, 2018 16:05

timdeschryver reviewed Aug 25, 2018

View reviewed changes

koumatsumoto force-pushed the add-logout-confirmation branch 2 times, most recently from 744d533 to 9a9373a Compare August 27, 2018 04:34

brandonroberts requested changes Aug 27, 2018

View reviewed changes

koumatsumoto force-pushed the add-logout-confirmation branch 2 times, most recently from 3f70aae to 5d0df04 Compare August 27, 2018 13:11

brandonroberts requested changes Aug 27, 2018

View reviewed changes

koumatsumoto added 7 commits August 28, 2018 09:46

feat(example): add logout confirmation

0504975

refactor(example): build stackblitz.html

2b5696c

refactor(example): remove unnecessary tests

27e3359

refactor(example): reorder import path

87c9834

refactor(example): use enum for dialog result message

081dc95

refactor(example): add a line-break refactor(example): fix incorrect value

update stackblitz.html

3a2c314

feat(example): add LogoutConfirmationDismiss action and use boolean t…

593f82a

…o dialog result

koumatsumoto force-pushed the add-logout-confirmation branch from 5d0df04 to 593f82a Compare August 28, 2018 00:46

koumatsumoto added 2 commits August 28, 2018 09:49

refactor(example): update dialog with css and clean test

fb60bc8

remove stackblitz.html

3ab14fc

brandonroberts approved these changes Aug 28, 2018

View reviewed changes

brandonroberts merged commit ba8d300 into ngrx:master Aug 28, 2018

koumatsumoto deleted the add-logout-confirmation branch September 2, 2018 12:06

Uh oh!

Conversation

koumatsumoto commented Aug 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

Uh oh!

coveralls commented Aug 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

timdeschryver Aug 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

koumatsumoto commented Aug 27, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

koumatsumoto commented Aug 27, 2018

Uh oh!

brandonroberts commented Aug 27, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

koumatsumoto commented Aug 28, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

koumatsumoto commented Aug 24, 2018 •

edited

Loading

coveralls commented Aug 24, 2018 •

edited

Loading

timdeschryver Aug 25, 2018 •

edited

Loading