Improved flyout demo & show iterator #2239
Conversation
Blackbaud-AlexKingman
changed the title
| }); | ||
|
|
||
| flyout.iteratorPreviousButtonClick | ||
| .takeUntil(this.openFlyoutStream) |
There was a problem hiding this comment.
You could use takeWhile in conjunction with a simple boolean property to reduce complexity.
.takeWhile(() => this.isFlyoutOpen)
There was a problem hiding this comment.
will do 👍
There was a problem hiding this comment.
I'm not sure if I can do this because isFlyoutOpen becomes false before the animation finishes. It causes some weird issues where the flyout won't ever fully open.
Is using .takeUntil() causing performance issues or is it just the complexity you're worried about?
Codecov Report
@@ Coverage Diff @@
## master #2239 +/- ##
=======================================
Coverage 98.34% 98.34%
=======================================
Files 22 22
Lines 121 121
Branches 12 12
=======================================
Hits 119 119
Misses 2 2Continue to review full report at Codecov.
|
Blackbaud-AlexKingman
changed the title
|
This is ready for another pass. All "rough" stuff has been refined and code is intended for final review. |
| selector: 'sky-flyout-demo-internal', | ||
| template: ` | ||
| <div class="sky-padding-even-large"> | ||
| <h2>This is an example of a simple flyout</h2> |
There was a problem hiding this comment.
Hmm, should this be real content instead of "This is an example..." ?
There was a problem hiding this comment.
Normally I would say no, but it seems like it would be valuable to have decriptions on some of these for what is special about the flyout. For example: the primary action one could say that the primary action is a button in the flyout header and if clicked it will execute something and then close the flyout.
I don't think it's necessary, but could be nice
| selector: 'sky-flyout-demo-internal', | ||
| template: ` | ||
| <div class="sky-padding-even-large"> | ||
| <h2>This is an example of a simple flyout</h2> |
There was a problem hiding this comment.
Normally I would say no, but it seems like it would be valuable to have decriptions on some of these for what is special about the flyout. For example: the primary action one could say that the primary action is a button in the flyout header and if clicked it will execute something and then close the flyout.
I don't think it's necessary, but could be nice
|
For the buttons with simple flyouts, could they be justified in some way? It's kind of weird to look through at the moment. The button labels could stand to be shorter too, but I'm not sure of good alternatives on that so maybe not. 😜 Otherwise, this demo looks fine to me for demonstrating flyouts and the nav-button functionality |
|
@blackbaud-conorwright ready for you again. I think I saw to every comment above, plus I organized the buttons to be a bit more tidy in the interest of legibility. |
There was a problem hiding this comment.
I think we should remove the "primary action" example, since the intended usage of that button in the header is to drill to the underlying content e.g. record or list page, which is covered by the permalink examples. General actions should be in a toolbar in the flyout view.
|
@Blackbaud-ToddRoberts @blackbaud-johnly Docs and UX changes have been made. Ready for final review. |
Added several examples of how to use flyout. Split complicated grid into its own demo to show how to use flyout with provider and iterator.
Addresses #2241