-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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(modal): [ngbAutofocus] option #2737
feat(modal): [ngbAutofocus] option #2737
Conversation
After discussing with @pkozlowski-opensource I will update PR to drop the config option So new rules are like so:
|
src/modal/modal-config.ts
Outdated
|
||
/** | ||
* Assign focus to the first focusable element when opened (true by default) | ||
* @since 3.3.0 |
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.
Please don't add the version since we don't know when it is going to be merged. I will add those when cutting a release.
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.
Ok, I will remove it !
src/modal/modal-config.ts
Outdated
* Assign focus to the first focusable element when opened (true by default) | ||
* @since 3.3.0 | ||
*/ | ||
focusFirst?: boolean; |
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.
My understanding was that we don't want to introduce this option for now. Did anything change?
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.
Did you see any new code push to this PR branch between my comment where I said I will remove it and now ? 😜
It will be removed...
src/modal/modal-config.ts
Outdated
@@ -75,4 +81,5 @@ export interface NgbModalOptions { | |||
export class NgbModalConfig implements NgbModalOptions { | |||
backdrop: boolean | 'static' = true; | |||
keyboard = true; | |||
focusFirst = 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.
My understanding was that we don't want to introduce this option for now. Did anything change?
src/modal/modal-stack.ts
Outdated
@@ -21,7 +21,8 @@ import {NgbModalWindow} from './modal-window'; | |||
|
|||
@Injectable({providedIn: 'root'}) | |||
export class NgbModalStack { | |||
private _windowAttributes = ['ariaLabelledBy', 'backdrop', 'centered', 'keyboard', 'size', 'windowClass']; | |||
private _windowAttributes = | |||
['ariaLabelledBy', 'backdrop', 'centered', 'keyboard', 'size', 'windowClass', 'focusFirst']; |
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.
My understanding was that we don't want to introduce this option for now. Did anything change?
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.
+1, you forgot to delete the focusFirst
option
src/util/focus-trap.ts
Outdated
import {Key} from '../util/key'; | ||
|
||
const FOCUSABLE_ELEMENTS_SELECTOR = [ | ||
export const FOCUSABLE_ELEMENTS_SELECTOR = [ |
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.
maybe it would be better to export a utility function instead?
54c09c5
to
f2055bb
Compare
@pkozlowski-opensource I made changes according to your review and also added a new demo for both explanation and illustration of the feature. |
src/modal/modal-window.ts
Outdated
@@ -62,7 +63,17 @@ export class NgbModalWindow implements OnInit, | |||
|
|||
ngAfterViewInit() { | |||
if (!this._elRef.nativeElement.contains(document.activeElement)) { | |||
this._elRef.nativeElement['focus'].apply(this._elRef.nativeElement, []); | |||
let elementToFocus = this._elRef.nativeElement.querySelector(`[ngbAutofocus]`) as HTMLElement; | |||
if (!elementToFocus) { |
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.
suggestion: Can you please merge this two level if statement, it looks maybe confusing (at least form me)
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.
Hey,
ok for me, but:
- stackblitz doesn't work
- you forgot the
focusFirst
option on modal
src/modal/modal-stack.ts
Outdated
@@ -21,7 +21,8 @@ import {NgbModalWindow} from './modal-window'; | |||
|
|||
@Injectable({providedIn: 'root'}) | |||
export class NgbModalStack { | |||
private _windowAttributes = ['ariaLabelledBy', 'backdrop', 'centered', 'keyboard', 'size', 'windowClass']; | |||
private _windowAttributes = | |||
['ariaLabelledBy', 'backdrop', 'centered', 'keyboard', 'size', 'windowClass', 'focusFirst']; |
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.
+1, you forgot to delete the focusFirst
option
f2055bb
to
aab1748
Compare
I am having a look at failing stackblitz |
aab1748
to
d4c23d9
Compare
misc/stackblitz-gen.js
Outdated
@@ -57,15 +58,16 @@ function generateIndexHtml() { | |||
return `<!DOCTYPE html> | |||
<html> | |||
|
|||
<head> | |||
<head>xx |
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.
xx?
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.
what the o_O ?? I have no clue why there are here, will remove them of course
<title>ng-bootstrap demo</title> | ||
<link rel="stylesheet" href="https://maxcdn.bootstrapcdn.com/bootstrap/${versions.bootstrap}/css/bootstrap.min.css" /> | ||
<link rel="stylesheet" href="https://cdnjs.cloudflare.com/ajax/libs/prism/1.15.0/themes/prism.css" /> |
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.
Why?
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.
misc/stackblitz-gen.js
Outdated
@@ -110,10 +112,10 @@ import { AppComponent } from './app.component'; | |||
import { ${demoImports} } from '${demoImport}'; | |||
|
|||
@NgModule({ | |||
imports: [BrowserModule, FormsModule, ReactiveFormsModule, HttpClientModule, NgbModule.forRoot()], | |||
imports: [BrowserModule, FormsModule, ReactiveFormsModule, HttpClientModule, NgbModule.forRoot()], |
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.
There are ws changes in this file and others - why?
BTW, I know it is not related to your change, but why do we have NgbModule.forRoot()
here?
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 never updated it i think... I will remove them
@@ -76,8 +81,7 @@ export class NgbModalWindow implements OnInit, | |||
} else { | |||
elementToFocus = body; | |||
} | |||
elementToFocus['focus'].apply(elementToFocus, []); | |||
|
|||
elementToFocus.focus(); |
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 know why we've called it through apply
before and don't do this here any more?
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 have no clue at all why you wrote all of these with apply()
. The only thing I know is that it seems to be not needed anymore. It works fine without it. So it makes the code easier to read
import {NgbModalBackdrop} from './modal-backdrop'; | ||
import {NgbModalWindow} from './modal-window'; | ||
import {NgbModal} from './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.
Why there are changes in this file?
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 have no clue... maybe formatting side effect....
[edit]: seems like a reordering of the import
simply decorate any element with this attribute to notify modal window to put focus on it when open.
d4c23d9
to
9337f27
Compare
LGTM |
As mentioned in #2728 this PR introduced 2 new features:
ability to use a new
ngbAutofocus
attribute in your modal templatesintroduction of a new
focusFirst
Modal Config option (will basically works asngbAutofocus
but finding automatically the first focusable element)Any newly opened modal will now do in that order:
ngbAutofocus
is detected, it will be used to focus the decorated element.focusFirst
value (default to betrue
), find the first focusable element, and focus it.focusFirst
set tofalse
) it will simply focus itself (the modal window).Another PR will be opened to demonstrate the usage of these new features in the Modal demos
Closes #2718