-
Notifications
You must be signed in to change notification settings - Fork 65
Flyout action button #1766
Flyout action button #1766
Conversation
Tests failed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: 2e18f43 (Please note that this is a fully automated comment.) |
Tests passed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED Commit: 027c1ee (Please note that this is a fully automated comment.) |
Codecov Report
@@ Coverage Diff @@
## master #1766 +/- ##
==========================================
+ Coverage 99.98% 99.98% +<.01%
==========================================
Files 410 410
Lines 8542 8556 +14
Branches 1253 1256 +3
==========================================
+ Hits 8541 8555 +14
Misses 1 1
Continue to review full report at Codecov.
|
I will be making a change to better support resize dragging when iframes are present on the page. Please do not review yet. |
…1754-flyout-action-button
…ckbaud-StewartStephens/skyux2 into ft-1754-flyout-action-button
OK, should be good to review now. |
Tests passed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED Commit: 660a08f (Please note that this is a fully automated comment.) |
Tests failed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: f8588bd (Please note that this is a fully automated comment.) |
Tests failed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: 3647d18 (Please note that this is a fully automated comment.) |
…ckbaud-StewartStephens/skyux2 into ft-1754-flyout-action-button
…ckbaud-StewartStephens/skyux2 into ft-1754-flyout-action-button
Tests failed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: 81e8a21 (Please note that this is a fully automated comment.) |
Tests failed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: f3788de (Please note that this is a fully automated comment.) |
Tests passed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED Commit: 6ea2424 (Please note that this is a fully automated comment.) |
Tests passed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED Commit: 3e78169 (Please note that this is a fully automated comment.) |
…ckbaud-StewartStephens/skyux2 into ft-1754-flyout-action-button
Tests failed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: 67e0a6b (Please note that this is a fully automated comment.) |
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.
@Blackbaud-StewartStephens Code looks great; just a few suggestions.
|
||
<ng-template #primaryActionTemplate> | ||
<ng-template [ngIf]="primaryAction && primaryAction.callback"> | ||
<a |
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.
Since the user may not be navigating to a route or URL with this button, we should instead represent this semantically as a button.
<button
class="sky-btn sky-btn-default sky-flyout-btn-primary-action"
type="button"
(click)="invokePrimaryAction()"
>
{{ primaryActionLabel }}
</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.
Switched this to a button.
@@ -62,6 +62,12 @@ | |||
} | |||
} | |||
|
|||
.sky-flyout-btn-primary-action { |
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 style won't be needed if we convert to using <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.
Removed this style. I left the class on the button element since the tests rely on using it as part of the selector.
|
||
// when iframes are present on the page, they may interfere with dragging | ||
// temporarily disable pointer events in iframes when dragging starts | ||
const iframes = document.querySelectorAll('iframe'); |
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'd like to research the iframe issue further (is there an issue filed for this?), but I don't want to hold up this pull request. Think we could pull this out into a different branch for now?
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 removed the iframe code from this branch and moved it to branch ch-flyout-iframe
in my fork. I also filed an issue #1794 to track it.
Tests failed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: 4ac1907 (Please note that this is a fully automated comment.) |
Tests failed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: 1e95385 (Please note that this is a fully automated comment.) |
Tests passed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED Commit: 0b78552 (Please note that this is a fully automated comment.) |
<ng-template [ngIf]="primaryAction && primaryAction.callback"> | ||
<button | ||
class="sky-btn sky-btn-default sky-flyout-btn-primary-action" | ||
href="#" |
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 type="button"
(if not provided, it defaults to "submit" which could potentially trigger a form submission).
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.
Added type="button"
.
@@ -39,6 +39,7 @@ import { | |||
SkyFlyoutMessage, | |||
SkyFlyoutMessageType | |||
} from './types'; | |||
import { SkyFlyoutAction } from './types/flyout-action'; |
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 should allow this type to be imported by consumers in their SPAs.
Need to include an export
statement here:
https://github.com/blackbaud/skyux2/blob/master/src/modules/flyout/types/index.ts
And here:
https://github.com/blackbaud/skyux2/blob/master/src/modules/flyout/index.ts#L9-L13
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.
Exported the type.
Tests passed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED Commit: 6dccbf3 (Please note that this is a fully automated comment.) |
Tests passed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED Commit: 7ba9856 (Please note that this is a fully automated comment.) |
Tests failed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: b117aef (Please note that this is a fully automated comment.) |
Tests failed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: 2810258 (Please note that this is a fully automated comment.) |
@blackbaud-sky-savage retry |
Tests passed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED Commit: 2810258 (Please note that this is a fully automated comment.) |
Created #2014 to document the flyout action |
Code change for issue #1754
Add primary action callback for the flyout component.