-
Notifications
You must be signed in to change notification settings - Fork 400
fix: Log in button works on first click #2591
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
Conversation
Fixes #1904
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.
The href
approach works for me if it works 🍰
I have one name change request and then some minor comments.
tests/unit/amo/helpers.js
Outdated
}); | ||
|
||
export function dispatchSignInActions({ | ||
export function dispatchAnonymousActions({ |
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 dispatchClientMetaData
would more accurately express what this is doing. It's simulating how the client will set its lang and user agent.
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.
wfm 👍
// https://github.com/mozilla/addons-frontend/issues/1904 | ||
// | ||
// For more info, see: https://css-tricks.com/annoying-mobile-double-tap-link-issue/#article-header-id-2 | ||
@media (pointer: 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.
Won't we still need something like this for all other buttons? I liked how it was a generic solution that would apply globally. If it doesn't make sense for this patch, maybe just file an issue.
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 kept testing it and it didn't really apply globally. It was very spotty and ultimately that element shouldn't have been an actual <button>
anyway if it did nothing without JS.
I liked it too but it seemed to work intermittently 😞
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.
Filed #2594
// mobile browser. This is the cause of | ||
// https://github.com/mozilla/addons-frontend/issues/1904 | ||
// | ||
// We tried to use https://css-tricks.com/annoying-mobile-double-tap-link-issue/#article-header-id-2 |
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 don't think you need to include this last sentence because it's more relevant to the development process and not the code itself. Keeping comments concise always helps since other devs don't always read comments.
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.
🚢
Fixes mozilla/addons#10170
This is frustrating but because the
<button>
element used for the "Log in to review this extension" button doesn't actually relate to anything the mobile browser will use its hover and focus states after first tap. It's a mess.The easiest fix is to put a
href="#"
on it and turn it into a link. That isn't great but it's the best we can do for now.