Skip to content
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

Enable autoFocus option to accept an element to focus on pane creation #18705

Merged
merged 5 commits into from Apr 27, 2019

Conversation

Projects
None yet
3 participants
@UziTech
Copy link
Contributor

commented Jan 16, 2019

Identify the Bug

fixes #18511

Description of the Change

Use panel.autoFocus as the element for focusTrap.initialFocus if it is not a boolean value.

Possible Drawbacks

None. It is backwards compaitible.

Verification Process

added a test

Release Notes

The autoFocus option for panels can now be an element that will receive initial focus.

@daviwil daviwil changed the title allow autoFocus option to be used as initialFocus on panel Enable autoFocus option to accept an element to focus on pane creation Jan 16, 2019

@daviwil
Copy link
Contributor

left a comment

Thanks @UziTech, this looks good! One possible change needed, but should be easy to merge this afterward.

})
}

if (panel.autoFocus !== true) {

This comment has been minimized.

Copy link
@daviwil

daviwil Jan 16, 2019

Contributor

What if someone deliberately sends undefined or null? It seems like those values will get through these two panel.autoFocus guards and possibly cause an error in focusTrap.

This comment has been minimized.

Copy link
@UziTech

UziTech Jan 16, 2019

Author Contributor

Nice catch! I updated it so any falsy value will be treated the same as false

@rafeca rafeca self-assigned this Apr 26, 2019

@rafeca

This comment has been minimized.

Copy link
Contributor

commented Apr 26, 2019

@UziTech: Thanks for contributing!

Since @daviwil already gave the 👍, I've just resolved the conflicts and I'll merge this once the CI tests pass 🎉

@rafeca rafeca merged commit 4fe8dbf into atom:master Apr 27, 2019

2 checks passed

Atom Pull Requests #20190427.5 succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.