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

Only target links with auth-link class #99

Closed
wants to merge 1 commit into from
Closed

Only target links with auth-link class #99

wants to merge 1 commit into from

Conversation

justinvoelker
Copy link

Pull request for #98

Rather than all anchor tags being included, only those with the default auth-link class will be handled via this logic.
@@ -28,7 +28,7 @@ jQuery(function($) {
return this.each(function() {
var $container = $(this);

$container.find('a').on('click', function(e) {
$container.find('a.auth-link').on('click', function(e) {
Copy link
Member

Choose a reason for hiding this comment

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

This would not work in case someone creates link with custom CSS class as auth-link added only in case no other class is specified.
https://github.com/yiisoft/yii2-authclient/blob/master/widgets/AuthChoice.php#L185

Copy link
Author

Choose a reason for hiding this comment

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

But if someone is already messing with the CSS classes, they can just add auth-link manually. This specific change would at least be be a non-breaking change that would still work for all existing widgets while allowing a certain level of control if needed.

Copy link
Member

Choose a reason for hiding this comment

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

clientLink() method should be adjusted as well.

Copy link
Author

Choose a reason for hiding this comment

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

What needs to be adjusted in clientLink()? I'm only seeing the one js file that controls what happens on click.

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