-
Notifications
You must be signed in to change notification settings - Fork 126
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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 15e5d96): https://fundamental-ngx-gh--pr12360-feat-popover-modal-u-yksgucte.web.app (expires Mon, 30 Sep 2024 17:59:52 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is supposed to say preventCloseOnEscapeKey & preventCloseOnOutsideClick
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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);
}
}`
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be nice if there was some way to close this after opening, back button maybe?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We also may want to see this removal logic as part of the destroy process for the service
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.
Couple more comments
87f85f6
to
a2544ea
Compare
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.
Two more small comments then we can merge it
// 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cleaner way to do this would be with the [focusTrapped]
input
] | ||
}) | ||
export class PopoverClosingExampleComponent { | ||
constructor() {} |
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.
no need for the empty constructor
a2544ea
to
b4b81ef
Compare
61c197b
to
884d9f0
Compare
884d9f0
to
db052e9
Compare
fixes #12210