-
Notifications
You must be signed in to change notification settings - Fork 6.8k
feat(material/expansion): allow focus origin to be optional input in focus method #20912
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(material/expansion): allow focus origin to be optional input in focus method #20912
Conversation
const header = fixture.debugElement.query(By.css('mat-expansion-panel-header'))!; | ||
const headerInstance = header.componentInstance; | ||
|
||
headerInstance.focus('mouse'); |
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.
This isn't the best test in my opinion. I attempted to focus the button with origin mouse
but this requires making this button a mat-button
in the component template below.
const button = fixture.debugElement.query(By.css('button'))!;
const buttonInstance = button.componentInstance as MatButton;
buttonInstance.focus('mouse');
This requires importing MatButton as well, which I was having trouble doing and was not sure if we should avoid importing other modules. If this is a better, however, can change it to this approach (assuming it works).
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 the way you have it here is fine
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
const header = fixture.debugElement.query(By.css('mat-expansion-panel-header'))!; | ||
const headerInstance = header.componentInstance; | ||
|
||
headerInstance.focus('mouse'); |
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 the way you have it here is fine
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. |
WHAT: For Angular Material components that have a focus() method, allow for the origin param to be optional and remove the default origin value.
WHY: For cases where the focus() method is called and the origin is already defined, we don’t want to override the origin using focusVia to always be some default value. In many cases, we want to leave the origin unchanged, but if there are cases that need the origin to be updated, allow for this with an optional origin param.