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

Add focusAfterToggle:boolean option to focus element after toggle #56

Conversation

dlegr250
Copy link
Contributor

@dlegr250 dlegr250 commented Aug 1, 2017

Awesome plugin, saved me from having to write my own!

I think most users will be focused in the text/password field entering input when they toggle the show/hide; I think it would be a good idea to allow an option to retain focus on the input element after toggling.

Default behavior does not change anything, must set the option in order for any effect to occur.

Did not update any version/scripts in case you all had a specific way you wanted to bump those.

// the element has been toggled. Benefitial for those who are
// typing out a password and just want to check their value
// and retain the focus of the input.
focusAfterToggle: false,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For consistency with the innerToggle option and for flexibility, should this instead be something like triggerOnToggle where you could provide an event name? For example, triggerOnToggle: 'focus' or triggerOnToggle: 'focus.my-focus-event'?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tylersticka I updated the PR using the triggerOnToggle syntax like you recommended to maintain consistency.

// Event to trigger whenever the element is toggled.
// For example, if 'focus' it will focus the cursor in the
// input element after toggling.
triggerOnToggle: '',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think this should still default to false? I know it means a conditional check later, but I don't know what the performance of running a needless trigger method is compared to checking for it first.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated PR with a default triggerOnToggle: false and a boolean check before calling the event. I'm not 100% sure on the performance either, but I assume that a boolean check is cheaper than an empty event call.

@tylersticka
Copy link
Member

@dlegr250 Thank you for making those changes! I'll merge this once I have some time to minify, version-bump and publish.

@tylersticka tylersticka merged commit 44f6509 into cloudfour:master Sep 11, 2017
@tylersticka
Copy link
Member

@dlegr250 Thanks again, available in 2.1.0!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants