feat(core): modal background styling #12360
Conversation
✅ Deploy Preview for fundamental-ngx ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
Visit the preview URL for this PR (updated for commit b4b81ef): https://fundamental-ngx-gh--pr12360-feat-popover-modal-u-ibnvs3ls.web.app (expires Sun, 22 Sep 2024 17:51:38 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: 41b993ee8e451bd7c6770b342ce142dc886eacff |
| [focusTrapped]="true" | ||
| > | ||
| <fd-popover-control> | ||
| <button fd-button>preventCloseOnEscapeKey & preventCloseOnEscapeKey</button> |
There was a problem hiding this comment.
I think this is supposed to say preventCloseOnEscapeKey & preventCloseOnOutsideClick
There was a problem hiding this comment.
But also maybe just go with typically formatted text i.e. "Prevent Close on Escape Key and Prevent Close on Outside Click" because the camel case on these buttons makes me think it's a property or something that the developer can set
There was a problem hiding this comment.
Couple comments above. Also, is it intentional that the opacity of the overlay is different than the opacity for dialog's overlay? I'm wondering if we needed to add a new class and new functionality, or if we can just borrow what's already being used for dialog
alexhristov14
force-pushed
the
feat/popover-modal-updated
branch
from
468d500
to
6b8832c
Compare
| @@ -230,12 +231,29 @@ export class PopoverService extends BasePopoverClass { | |||
| } | |||
| } | |||
|
|
|||
| /** Changes background theming when modal */ | |||
| checkModalBackground(): void { | |||
There was a problem hiding this comment.
Add unit tests for checkModalBackground()
| @@ -230,12 +231,29 @@ export class PopoverService extends BasePopoverClass { | |||
| } | |||
| } | |||
|
|
|||
| /** Changes background theming when modal */ | |||
| checkModalBackground(): void { | |||
| if ((!this.closeOnOutsideClick || !this.closeOnEscapeKey) && this.isOpen) { | |||
There was a problem hiding this comment.
Consider reducing the repetition in adding/removing classes by consolidating logic where applicable.
for example:
`checkModalBackground(): void {
const bodyClass = 'fd-overlay-active';
const triggerClass = 'fd-popover__modal';
if ((!this.closeOnOutsideClick || !this.closeOnEscapeKey) && this.isOpen) {
this.addClasses(bodyClass, triggerClass);
} else {
this.removeClasses(bodyClass, triggerClass);
}
}`
alexhristov14
force-pushed
the
feat/popover-modal-updated
branch
from
6b8832c
to
2469c56
Compare
| [focusTrapped]="true" | ||
| > | ||
| <fd-popover-control> | ||
| <button fd-button>Prevent Close on Outside Click & on Escape Key</button> |
There was a problem hiding this comment.
Would be nice if there was some way to close this after opening, back button maybe?
There was a problem hiding this comment.
You can close by clicking the popover again. That's why I didn't overlay the button so that it shows that it's intractable even with the closing properties
| <div class="fd-docs-flex-display-helper"> | ||
| <fd-popover [closeOnOutsideClick]="false" [focusAutoCapture]="true" [focusTrapped]="true"> | ||
| <fd-popover-control> | ||
| <button fd-button>Prevent Close on Outside Click</button> |
There was a problem hiding this comment.
If I open this example, then click somewhere on the background thus de-focusing the back button, then pressing escape does not close the popover
| if ((!this.closeOnOutsideClick || !this.closeOnEscapeKey) && this.isOpen) { | ||
| this._renderer.addClass(document.body, bodyClass); | ||
| this._renderer.addClass((this._triggerElement as ElementRef).nativeElement, triggerClass); | ||
| } else if ((!this.closeOnOutsideClick || !this.closeOnEscapeKey) && !this.isOpen) { |
There was a problem hiding this comment.
We also may want to see this removal logic as part of the destroy process for the service
alexhristov14
force-pushed
the
feat/popover-modal-updated
branch
from
87f85f6
to
a2544ea
Compare
| // Preventing the focus from leaving the popover body button when the popover is open | ||
|
|
||
| ngOnInit(): void { | ||
| document.addEventListener('mousedown', this.keepFocusOnButton.bind(this)); |
There was a problem hiding this comment.
Cleaner way to do this would be with the [focusTrapped] input
| ] | ||
| }) | ||
| export class PopoverClosingExampleComponent { | ||
| constructor() {} |
There was a problem hiding this comment.
no need for the empty constructor
alexhristov14
force-pushed
the
feat/popover-modal-updated
branch
from
a2544ea
to
b4b81ef
Compare
fixes #12210