Full-page modal, add class name to body tag
#1180
Conversation
…voker on full page modals
| /deep/ | ||
| .sky-modal-body-full-page { | ||
| // Hide the bb-Help-invoker when a full-page modal is present and the widget is closed. | ||
| #bb-help-container.closed > #bb-help-invoker { |
There was a problem hiding this comment.
The style overwrite should be added to the help library, to keep things separated. This branch should only be concerned with adding the "full page" body class.
There was a problem hiding this comment.
I disagree, the help widget shouldn't care about the SKY UX style requirements. This would be an example of SKY UX setting a style requirement override for using an element from this library, and should handle that override itself. If SKY were to suddenly change it's style requirements or UX requirements, it wouldn't make sense to have to update a third party library to accommodate that change. It should be agnostic and unaware of SKY.
|
Tests passed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED Commit: 8dd19b9 (Please note that this is a fully automated comment.) |
Codecov Report
@@ Coverage Diff @@
## master #1180 +/- ##
======================================
Coverage 100% 100%
======================================
Files 355 355
Lines 6604 6622 +18
Branches 852 853 +1
======================================
+ Hits 6604 6622 +18
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: 8ea6edd (Please note that this is a fully automated comment.) |
Blackbaud-SteveBrush
changed the title
body tag
| public setPageScroll(isAdd: boolean, isFullPage: boolean): void { | ||
| const modalBodyClasses = ['sky-modal-body-open']; | ||
|
|
||
| if (isFullPage) { |
There was a problem hiding this comment.
The logic here needs to be refined. Think of this example:
- User opens full-page modal, so
isFullPageistrue. Thefull-pageCSS class is added. - User opens another full-page modal, so
isFullPageistrue. Thefull-pageCSS class is added (though not really because it already exists in thebody.classList). - User closes the second full-page modal, so
isFullPageistrue. Thefull-pageCSS class is removed. - There is now a full-page modal still open but no
full-pageCSS class on thebodyelement.
You'll need to track the count of full-page modals separately from the count of all modals and only remove the full-page CSS class when the last full-page modal is closed.
There was a problem hiding this comment.
To add to Paul's comments, I feel like this method is not the best place to be adding unrelated body classes since it's specialized for a specific purpose.
| // https://bugzilla.mozilla.org/show_bug.cgi?id=814014 | ||
| modalBodyClasses.forEach(bodyClass => { | ||
| document.body.classList.add(bodyClass); | ||
| }); |
There was a problem hiding this comment.
You could avoid the loops and the classList entirely by using Angular's Renderer2.addClass method. https://angular.io/api/core/Renderer2
| document.body.classList.remove(modalClass); | ||
| modalBodyClasses.forEach(bodyClass => { | ||
| document.body.classList.remove(bodyClass); | ||
| }); |
There was a problem hiding this comment.
Same here, but with removeClass.
|
Tests failed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: f19d23d (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: bef2785 (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: be58f8a (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: c81020d (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: bb9be96 (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: f8e3574 (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: 7bc967c (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: 2cd94cf (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: b1b1214 (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: de3d5d3 (Please note that this is a fully automated comment.) |
blackbaud/skyux-lib-help#12
The
xbutton was hidden on full page modals, according to UX the Invoker tab should be hidden on full page modals only.Added a full page body class for full page modals, hiding the help invoker on full page modals