-
Notifications
You must be signed in to change notification settings - Fork 65
Conversation
@@ -384,10 +384,35 @@ <h3> | |||
<button | |||
type="button" | |||
class="sky-btn sky-btn-default" | |||
[skyPopover]="asyncPopoverRef"> | |||
[skyPopover]="asyncPopover"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this still needs to reference the asyncPopoverRef
variable from the component.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
type="button" | ||
class="sky-btn sky-btn-default" | ||
(click)="closePopover()"> | ||
Close popover |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As you mentioned offline, the popover always closes when you click outside the popover. To better demonstrate the close, could you simple place this button inside the popover?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we really want to do that as I don't know that popovers are normally intended for buttons. However, we could do something where it opens and then uses a timer to close after a certain amount of time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. This has been modified.
<button | ||
type="button" | ||
class="sky-btn sky-btn-default" | ||
[skyPopover]="programaticPopoverRef" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does putting the programmatic popover on a button with the directive on it kind of defeat the purpose? This button will work regardless of the skyPopoverMessageStream
input var. Is there a better way we could show a programatic example?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has been modified.
Created #1986 to document new input |
@blackbaud-conorwright @Blackbaud-AlexKingman I've pushed another small change to the demo. Let me know if you have anything else here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs to be addressed in a different pull request.
@@ -391,3 +391,28 @@ <h3> | |||
<sky-popover #asyncPopover> | |||
My asynchronous popover. | |||
</sky-popover> | |||
|
|||
<h3> | |||
Popovers may be interacted with programatically |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"programmatically"
|
||
public openPopover() { | ||
this.sendMessage(SkyPopoverMessageType.Open); | ||
setTimeout(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is causing some strange behavior in the demo. I would isolate this to two separate buttons, "Open" and "Close".
<button | ||
type="button" | ||
class="sky-btn sky-btn-default" | ||
[skyPopover]="programaticPopover" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"programmatic"
type="button" | ||
class="sky-btn sky-btn-default" | ||
(click)="openPopover()"> | ||
Open popover programatically |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"programmatically"
</button> | ||
</p> | ||
<sky-popover #programaticPopover> | ||
This popover has a programatic trigger on the second button. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"programmatic"
/* tslint:disable-next-line:switch-default */ | ||
switch (message.type) { | ||
case SkyPopoverMessageType.Open: | ||
this.positionPopover(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to make sure that the popover itself uses the message stream to open/close/position so that consumers can interrupt the message stream as they see fit.
For example, you would replace this.positionPopover()
, here:
this.positionPopover(); |
...with
this.sendMessage(SkyPopoverMessageType.Open)
;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dropdown does it this way too:
https://github.com/blackbaud/skyux2/blob/master/src/modules/dropdown/dropdown.component.ts#L220-L222
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Issue created: #1994
Resolves #1839