Added row iterator properties #10

Blackbaud-AlexKingman · 2018-12-18T18:03:27Z

codecov-io · 2018-12-18T18:36:06Z

Codecov Report

Merging #10 into master will increase coverage by 0.07%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master      #10      +/-   ##
==========================================
+ Coverage   99.64%   99.71%   +0.07%     
==========================================
  Files           9       10       +1     
  Lines         280      350      +70     
  Branches       42       53      +11     
==========================================
+ Hits          279      349      +70     
  Misses          1        1

Impacted Files	Coverage Δ
src/app/public/modules/flyout/flyout.module.ts	`100% <100%> (ø)`	⬆️
...public/modules/flyout/types/flyout-message-type.ts	`100% <100%> (ø)`	⬆️
src/app/public/modules/flyout/flyout.component.ts	`100% <100%> (ø)`	⬆️
src/app/public/modules/flyout/flyout-instance.ts	`100% <100%> (ø)`	⬆️
...public/modules/flyout/flyout-iterator.component.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 f85ff15...f2ef418. Read the comment docs.

Blackbaud-SteveBrush · 2018-12-18T19:52:23Z

src/app/visual/flyout/flyout-visual.component.ts

+      }
+    });
+
+    this.flyout.onRowIteratorPreviousClick.subscribe(() => {


The prefix "on" should be used for the listener, not the event itself. In this case, consumers are subscribing to an observable event, so the name shouldn't be prefixed with "on".

(Also, I know this feature is going to be used by grid rows, but we probably shouldn't include the "row" keyword to the event name. To me, it communicates an enforced pattern that isn't always applicable. What if the buttons iterated through a list or a group of action buttons?)

Something like the following?

this.flyout.previousIteratorButtonClick.subscribe(...) // or... this.flyout.iteratorPreviousButtonClick.subscribe(...) // or... this.flyout.iterator.previousButtonClick.subscribe(...)

(That last example matches the config pattern, if you wanted to go that route.)

The visual test (and the future demo) should also demonstrate that these subscriptions need to be completed in ngOnDestroy.

Good call about the "row" keyword. I've removed all references to "row" in the codebase. I've also gone with the this.flyout.iterator.previousButtonClick pattern.

Blackbaud-SteveBrush · 2018-12-18T20:00:09Z

src/app/visual/flyout/flyout-visual.component.ts

+        useValue: record
+      }],
+      rowIterator: {
+        previousIsDisabled: previousIsDisabled,


previousButtonDisabled

It might be easier for people to remember the property name if we use the pattern set by the HTML standard for the disabled attribute.

this is done

Blackbaud-SteveBrush · 2018-12-18T20:00:37Z

src/app/visual/flyout/flyout-visual.component.ts

@@ -3,7 +3,7 @@ import {
 } from '@angular/core';

 import {
-  SkyFlyoutService
+  SkyFlyoutService, SkyFlyoutInstance


Multiline import.

src/app/public/modules/flyout/flyout-instance.ts

Blackbaud-SteveBrush · 2018-12-19T15:45:11Z

src/app/public/modules/flyout/types/flyout-iterator.ts

@@ -0,0 +1,4 @@
+export interface SkyFlyoutIterator {


Maybe we should rename this to SkyFlyoutIteratorConfig so that we can use the SkyFlyoutIterator type for the public SkyFlyoutInstance.iterator property?

Blackbaud-SteveBrush · 2018-12-19T15:47:05Z

src/app/public/modules/flyout/flyout.component.html

+    <div class="sky-flyout-header-content">
+
+      <div *ngIf="config?.iterator"
+        id="row-iterators"


Curious why you need an id on this element? If so, it will need an auto-increment applied to keep it unique.

It was meant as a unique identifier to easily locate the buttons in the unit test. Isn't there only ever one flyout open at a time?

Blackbaud-SteveBrush · 2018-12-19T15:48:40Z

src/app/public/modules/flyout/flyout.component.html

+      </div>
+
+    </div>
+      <div class="sky-flyout-header-buttons">


There might be a few extra tabs in the template, here.

Blackbaud-SteveBrush · 2018-12-19T15:51:38Z

src/app/public/modules/flyout/flyout.component.ts

@@ -251,6 +251,18 @@ export class SkyFlyoutComponent implements OnDestroy, OnInit {
    this.adapter.toggleIframePointerEvents(true);
  }

+  public onIteratorPreviousClick() {
+    if (this.config.iterator && !this.config.iterator.previousButtonDisabled) {


Since these methods are isolated to the buttons' click events, do we need the conditions? For these events to be executed, the iterator configuration must already exist, and the event won't be triggered if the button is natively disabled. You might be safe to simply call the emit methods without any condition.

Yeah true, but these are public methods. I think I was just adding extra safeguards here in case they were used for something else in the future. Could I persuade you on this one in the interest of being extra cautious?

Blackbaud-SteveBrush · 2018-12-19T16:21:17Z

src/app/visual/flyout/flyout-visual.component.ts

 } from '@angular/core';

 import {
+  Subject
+} from 'rxjs';


Use 'rxjs/Subject' until we can update to the latest version of RxJS (they've improved their imports in later versions, and don't pull in more than they ought to).

Blackbaud-TimHammond · 2018-12-19T17:29:41Z

The next row/previous row buttons look good from a UX perspective.

…opened

Blackbaud-SteveBrush · 2018-12-20T18:43:54Z

src/app/public/modules/flyout/flyout.module.ts

@@ -60,7 +65,8 @@ import {
    SkyFlyoutResourcesModule
  ],
  exports: [
-    SkyFlyoutComponent
+    SkyFlyoutComponent,
+    SkyFlyoutIteratorComponent


We probably don't want to export the iterator component to the consumers.

Whoops. That was an oversight. Fixed.

Blackbaud-SteveBrush · 2018-12-20T18:47:11Z

src/app/public/modules/flyout/flyout-instance.ts

  public closed = new EventEmitter<void>();
  public componentInstance: T;
  public isOpen = true;

+  public iteratorPreviousButtonClick = new EventEmitter<void>();


The EventEmitter instance variables should be getters, so that the consumer cannot reassign them.

Blackbaud-SteveBrush · 2018-12-20T18:51:59Z

src/app/public/modules/flyout/flyout.component.ts

+  }
+
+  public onIteratorNextButtonClick() {
+    if (this.config.showIterator && !this.config.iteratorNextButtonDisabled) {


These events should never be emitted if the underlying button is disabled, so isn't the this.config.iteratorNextButtonDisabled check redundant? (Same for the previous button.)

Blackbaud-SteveBrush · 2018-12-20T18:53:49Z

src/app/public/modules/flyout/types/flyout-message-type.ts

@@ -1,4 +1,8 @@
 export enum SkyFlyoutMessageType {
  Open = 0,
-  Close = 1
+  Close = 1,
+  IteratorNextButtonEnabled = 10,


Message types should be verbs, EnableIteratorNextButton. Also, you can let the enum auto-increment the IDs by writing it this way:

export enum SkyFlyoutMessageType { Open = 0, Close, EnableIteratorNextButton, ... }

good tip. Done.

Blackbaud-SteveBrush

Beautiful.

initial commit for row iterators

0c5c57b

Blackbaud-AlexKingman requested a review from Blackbaud-SteveBrush as a code owner December 18, 2018 18:03

resetting baselines

ad98d5e

Blackbaud-SteveBrush reviewed Dec 18, 2018

View reviewed changes

Blackbaud-AlexKingman added 5 commits December 18, 2018 17:08

pr feedback

8d49a35

using ngUnsubscribe

3e3df1e

fixed linting error

715d850

added aria labels

fa77842

added test coverage

c7c5d9f

Blackbaud-SteveBrush reviewed Dec 19, 2018

View reviewed changes

Blackbaud-AlexKingman added 3 commits December 19, 2018 15:48

created FlyoutIteratorComponent

840783b

added message types for enabling/disabling iterators after flyout is …

e708f0b

…opened

whitespace

4b7a961

Blackbaud-SteveBrush reviewed Dec 20, 2018

View reviewed changes

Blackbaud-AlexKingman added 2 commits December 20, 2018 14:35

pr feedback

8e4a142

removed if checks

f2ef418

Blackbaud-SteveBrush approved these changes Dec 20, 2018

View reviewed changes

Blackbaud-AlexKingman merged commit fa8a51c into master Dec 20, 2018

Blackbaud-SteveBrush deleted the contrib-row-iterator branch December 20, 2018 20:37

This was referenced Dec 21, 2018

Flyout > Add row iterators to header blackbaud/skyux2#2143

Closed

Document flyout iterators blackbaud/skyux2#2237

Closed

This was referenced Jan 10, 2019

Apply highlight animation based on input condition blackbaud/skyux-grids#20

Closed

changes related to highlighted row based on condition blackbaud/skyux-grids#21

Closed

Changes related to iteration support #8

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added row iterator properties #10

Added row iterator properties #10

Blackbaud-AlexKingman commented Dec 18, 2018 •

edited

Loading

codecov-io commented Dec 18, 2018 •

edited

Loading

Blackbaud-SteveBrush Dec 18, 2018

Blackbaud-SteveBrush Dec 18, 2018

Blackbaud-AlexKingman Dec 18, 2018

Blackbaud-SteveBrush Dec 18, 2018

Blackbaud-AlexKingman Dec 18, 2018

Blackbaud-SteveBrush Dec 18, 2018

Blackbaud-AlexKingman Dec 18, 2018

Blackbaud-SteveBrush Dec 19, 2018

Blackbaud-SteveBrush Dec 19, 2018

Blackbaud-AlexKingman Dec 19, 2018

Blackbaud-SteveBrush Dec 19, 2018

Blackbaud-SteveBrush Dec 19, 2018

Blackbaud-AlexKingman Dec 19, 2018

Blackbaud-SteveBrush Dec 19, 2018

Blackbaud-AlexKingman Dec 19, 2018

Blackbaud-TimHammond commented Dec 19, 2018

Blackbaud-SteveBrush Dec 20, 2018

Blackbaud-AlexKingman Dec 20, 2018

Blackbaud-SteveBrush Dec 20, 2018

Blackbaud-AlexKingman Dec 20, 2018

Blackbaud-SteveBrush Dec 20, 2018

Blackbaud-SteveBrush Dec 20, 2018

Blackbaud-AlexKingman Dec 20, 2018

Blackbaud-SteveBrush left a comment

Added row iterator properties #10

Added row iterator properties #10

Conversation

Blackbaud-AlexKingman commented Dec 18, 2018 • edited Loading

codecov-io commented Dec 18, 2018 • 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

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

Blackbaud-TimHammond commented Dec 19, 2018

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

Blackbaud-SteveBrush left a comment

Choose a reason for hiding this comment

Blackbaud-AlexKingman commented Dec 18, 2018 •

edited

Loading

codecov-io commented Dec 18, 2018 •

edited

Loading