Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion src/core/components/AuthenticateButton/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -60,8 +60,14 @@ export class AuthenticateButtonBase extends React.Component {
const buttonText = isAuthenticated ?
logOutText || i18n.gettext('Log out') :
logInText || i18n.gettext('Log in/Sign up');

// The `href` is required because a <button> element with a :hover effect
// and/or focus effect (that is not part of a form) that changes its
// appearance will transition to the hover state onClick when using a
// mobile browser. This is the cause of
// https://github.com/mozilla/addons-frontend/issues/1904
return (
<Button onClick={this.onClick} {...otherProps}>
<Button onClick={this.onClick} href="#" {...otherProps}>
{noIcon ? null : <Icon name="user-dark" />}
{buttonText}
</Button>
Expand Down
13 changes: 2 additions & 11 deletions src/core/css/inc/mixins.scss
Original file line number Diff line number Diff line change
Expand Up @@ -219,17 +219,8 @@ $default-arrow-margin: 3px;
font-weight: 400;
}

// This is required because a <button> element with a :hover effect
// that changes its appearance will transition to the hover state onClick
// when using a mobile browser.
// This is the cause of
// 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) {
Copy link
Contributor

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.

Copy link
Contributor Author

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 😞

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Filed #2594

&:hover {
background: $background-hover;
}
&:hover {
background: $background-hover;
}

&:active,
Expand Down
19 changes: 16 additions & 3 deletions tests/unit/amo/helpers.js
Original file line number Diff line number Diff line change
Expand Up @@ -66,17 +66,30 @@ export const signedInApiState = Object.freeze({
clientApp: 'firefox',
});

export function dispatchSignInActions({
export function dispatchClientMetadata({
store = createStore().store,
clientApp = 'android',
authToken = userAuthToken(),
lang = 'en-US',
userAgent = sampleUserAgent,
} = {}) {
store.dispatch(setAuthToken(authToken));
store.dispatch(setClientApp(clientApp));
store.dispatch(setLang(lang));
store.dispatch(setUserAgent(userAgent));

return {
store,
state: store.getState(),
};
}

export function dispatchSignInActions({
authToken = userAuthToken(),
...otherArgs
} = {}) {
const { store } = dispatchClientMetadata(otherArgs);

store.dispatch(setAuthToken(authToken));

return {
store,
state: store.getState(),
Expand Down
25 changes: 16 additions & 9 deletions tests/unit/core/components/TestAuthenticateButton.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import {
Simulate, findRenderedComponentWithType, renderIntoDocument,
} from 'react-addons-test-utils';
import { findDOMNode } from 'react-dom';
import { combineReducers, createStore as _createStore } from 'redux';
import { Provider } from 'react-redux';

import { setAuthToken } from 'core/actions';
import * as api from 'core/api';
Expand All @@ -13,17 +13,24 @@ import {
mapDispatchToProps,
mapStateToProps,
} from 'core/components/AuthenticateButton';
import apiReducer from 'core/reducers/api';
import {
dispatchClientMetadata,
dispatchSignInActions,
} from 'tests/unit/amo/helpers';
import { getFakeI18nInst, userAuthToken } from 'tests/unit/helpers';
import Icon from 'ui/components/Icon';

function createStore() {
return { store: _createStore(combineReducers({ api: apiReducer })) };
}

describe('<AuthenticateButton />', () => {
const renderTree = (props) => renderIntoDocument(
<AuthenticateButtonBase i18n={getFakeI18nInst()} {...props} />);
function renderTree(props) {
const { store } = dispatchSignInActions();

return findRenderedComponentWithType(renderIntoDocument(
<Provider store={store}>
<AuthenticateButtonBase i18n={getFakeI18nInst()} {...props} />
</Provider>
), AuthenticateButtonBase);
}

const render = (props) => findDOMNode(renderTree(props));

Expand Down Expand Up @@ -93,7 +100,7 @@ describe('<AuthenticateButton />', () => {
};
sinon.stub(config, 'get', (key) => _config[key]);

const { store } = createStore();
const { store } = dispatchSignInActions();
store.dispatch(setAuthToken(userAuthToken({ user_id: 99 })));
const apiConfig = { token: store.getState().api.token };
expect(apiConfig.token).toBeTruthy();
Expand All @@ -107,7 +114,7 @@ describe('<AuthenticateButton />', () => {
});

it('pulls isAuthenticated from state', () => {
const { store } = createStore(combineReducers({ api }));
const { store } = dispatchClientMetadata();
expect(mapStateToProps(store.getState()).isAuthenticated).toEqual(false);
store.dispatch(setAuthToken(userAuthToken({ user_id: 123 })));
expect(mapStateToProps(store.getState()).isAuthenticated).toEqual(true);
Expand Down