-
Notifications
You must be signed in to change notification settings - Fork 6.8k
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(FocusOriginMonitor): support monitoring subtree focus as well as element #3113
Conversation
src/lib/core/style/focus-classes.ts
Outdated
// Create monitored element info. | ||
let info: MonitoredElementInfo = { | ||
unlisten: null, | ||
includesChildren: includeChildren, |
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.
How about checkChildren
?
src/lib/core/style/focus-classes.ts
Outdated
// focused element. | ||
window.addEventListener('focus', () => { | ||
this._windowFocused = true; | ||
setTimeout(() => this._windowFocused = false, 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.
Just an optional nit. How about extracting this into its own function?
constructor(ngZone: NgZone) {
ngZone.runOutsideAngular(() => this._registerDocumentEvents());
}
This way we would get rid of unnecessary indentation and also have it pretty clean.
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.
LGTM. One small nit.
src/lib/core/style/focus-classes.ts
Outdated
|
||
// If we are not counting child-element-focus as focused, make sure that the event target is the | ||
// monitored element itself. | ||
if (!this._elementInfo.get(element).checkChildren && element != event.target) { |
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 that we should use ===
as often as possible. Also in the setClasses
method.
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Also includes some cleanup:
registerElementForFocusClasses
tomonitor
unmonitor
method and call on directive destroyedcdkFocusClasses
withcdkMonitorElementFocus
andcdkMonitorSubtreeFocus