-
Notifications
You must be signed in to change notification settings - Fork 65
Add tiledBody option to modal component. #1039
Add tiledBody option to modal component. #1039
Conversation
Tests failed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: cf7e129 (Please note that this is a fully automated comment.) |
Codecov Report
@@ Coverage Diff @@
## master #1039 +/- ##
======================================
Coverage 100% 100%
======================================
Files 317 317
Lines 5976 5980 +4
Branches 759 759
======================================
+ Hits 5976 5980 +4
Continue to review full report at Codecov.
|
Tests passed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED Commit: efa00e8 (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: 51aac99 (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: e9550bf (Please note that this is a fully automated comment.) |
Fixed typo of SkyModalConfigurationInterface and exporting in default…
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.
Looks good from the UX side
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 feel better if we also had a visual test to represent the tiled body.
Tests failed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: bae5229 (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: 50bed43 (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: 7aab722 (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: bddaa91 (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: 4e2b86c (Please note that this is a fully automated comment.) |
export class ModalTiledDemoComponent implements OnInit { | ||
public title = 'Hello world'; | ||
|
||
public ngOnInit() { |
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.
Do you need to include ngOnInit
for the test?
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-SteveBrush I was just mirroring the other tests -do you want me to remove it from all of the modal tests?
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.
Huh, you're right. Don't worry about it, we can clean these up in a different PR.
@@ -0,0 +1,26 @@ | |||
<sky-modal [tiledBody]=true> |
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.
[tiledBody]="true"
src/app/components/modal/index.html
Outdated
defaultValue="false" | ||
isOptional="true" | ||
> | ||
When set to true, the modal will be styled for use with sky-tile sections. |
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-johnly: Got any feedback on the docs portion?
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 reword this to be consistent with other descriptions for Boolean properties: "Indicates whether to style the modal for use with sky-tile
sections." Or more simply, "Indicates whether the modal uses tiles." (Formatting "sky-style
" as a code snippet, and doing the same with "true
" if we don't reword this.)
Also, I think it would be useful to flesh this out. Based on the description in the issue this addresses (#1038), I'd suggest a second sentence like: "When set to true
, the modal's background switches to $sky-background-color-neutral-light, and tile headings are styled as subsection headings."
@@ -0,0 +1,32 @@ | |||
<sky-modal [tiledBody]=true> |
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.
[tiledBody]="true"
Tests failed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: 37eed29 (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: 56cdba7 (Please note that this is a fully automated comment.) |
Visual test looks good. Waiting for @blackbaud-johnly review.
Tests failed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: 047d745 (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: 317675f (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: b0e0e34 (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: 8de4041 (Please note that this is a fully automated comment.) |
<button type="button" class="sky-btn sky-btn-secondary" (click)="openModal('fullScreenModal')"> | ||
Open Full Screen modal | ||
<button type="button" class="sky-btn sky-btn-primary" (click)="openModal('fullScreenModal')"> | ||
Open full screen modal |
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.
"full-screen" should be hyphenated
Tests failed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: 0d069cf (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: 0b548f6 (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: 0fae2cd (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: 00f555d (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: 4ae5422 (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: 788fede (Please note that this is a fully automated comment.) |
Issue #1038