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

Fix #9 add localization to login component using react-intl package #22

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
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
21 changes: 12 additions & 9 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -59,15 +59,6 @@
"morgan": "^1.7.0",
"node-sass": "^3.12.4",
"postcss-loader": "^1.1.1",
"react": "^15.4.0",
"react-addons-test-utils": "^15.4.1",
"react-css-themr": "^1.6.0",
"react-dom": "^15.4.0",
"react-hot-loader": "^1.3.1",
"react-redux": "^4.4.6",
"react-router": "^3.0.0",
"redux": "^3.6.0",
"redux-thunk": "^2.1.0",
"rimraf": "^2.5.4",
"sass-loader": "^4.0.2",
"style-loader": "^0.13.1",
Expand All @@ -76,5 +67,17 @@
"webpack": "^1.13.3",
"webpack-dev-middleware": "^1.8.4",
"webpack-hot-middleware": "^2.13.2"
},
"dependencies": {
"react-intl": "^2.2.3",
"react": "^15.4.0",
"react-addons-test-utils": "^15.4.1",
"react-css-themr": "^1.6.0",
"react-dom": "^15.4.0",
"react-hot-loader": "^1.3.1",
"react-redux": "^4.4.6",
"react-router": "^3.0.0",
"redux": "^3.6.0",
"redux-thunk": "^2.1.0"
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we have all of these external dependencies all of a sudden? We shouldn't need any of these... Specifically, our only external dependency would be React, but we webpack everything up when we build, so our exports can be used anywhere regardless of environment.

}
49 changes: 49 additions & 0 deletions src/Form/Input/index.jsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
import React, {
Component,
PropTypes
} from 'react';

import {
intlShape,
injectIntl
} from 'react-intl';
Copy link
Contributor

Choose a reason for hiding this comment

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

Not really a fan of this style except when you are importing a lot of items. We should decide if we like this and consider allowing/disallowing it in our linting rules.




class Input extends Component {
render() {
const { type, intl, value, required, id, onChange, placeholder } = this.props;
const props = {
type,
value,
required,
id,
onChange
};
if (placeholder) {
const { formatMessage } = intl;
props.placeholder = formatMessage(placeholder);
}
return (
<input
{...props}
/>
);
}
}

Input.propTypes = {
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be static propTypes = {...} inside the class.

Copy link
Collaborator

@dennismphil dennismphil Feb 21, 2017

Choose a reason for hiding this comment

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

Can you use the static PropType syntax here

type: PropTypes.string,
intl: intlShape.isRequired,
value: PropTypes.oneOfType([PropTypes.string, PropTypes.bool]),
required: PropTypes.bool,
id: PropTypes.string,
onChange: PropTypes.func,
placeholder: PropTypes.object,
};

Input.defaultProps = {
Copy link
Contributor

Choose a reason for hiding this comment

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

static defaultProps = {..}

inputType: 'text'
};

export default injectIntl(Input);
63 changes: 0 additions & 63 deletions src/Login/index.js

This file was deleted.

148 changes: 148 additions & 0 deletions src/Login/index.jsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,148 @@
import React, { Component, PropTypes } from 'react';
Copy link
Contributor

Choose a reason for hiding this comment

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

Currently not putting .jsx extensions on code. If we want to, we should be consistent about it. I do not have a strong opinion on whether we should or should not.

import { themr } from 'react-css-themr';
import { connect } from 'react-redux';
import {
addLocaleData,
IntlProvider,
FormattedMessage,
defineMessages
} from 'react-intl';
import enLocaleData from 'react-intl/locale-data/en';
import zhLocaleData from 'react-intl/locale-data/zh';

import { setUsername, setPassword, toggleRemember, attemptLogin } from './actions';
import Input from '../Form/Input';
import style from './style.scss';
import constants from '../constants';
import zhMessage from './l10n/zh_CN';
import enMessage from './l10n/en_US';

addLocaleData(enLocaleData);
addLocaleData(zhLocaleData);
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we bundling all of the locale data inside webpack? Won't this make our exports huge?


const getLocaleMessage = (locale) => {
switch (locale) {
case 'en':
return enMessage;
case 'zh':
return zhMessage;
}
}

const placeholderMessages = defineMessages({
username: {
id: 'placeholder.username'
},
password: {
id: 'placeholder.password'
}
})

class Login extends Component {
static propTypes = {
theme: PropTypes.object.isRequired,
username: PropTypes.string,
password: PropTypes.string,
remember: PropTypes.bool,
onUsernameChange: PropTypes.func,
onPasswordChange: PropTypes.func,
onRememberToggle: PropTypes.func,
onLogin: PropTypes.func,
Copy link
Collaborator

Choose a reason for hiding this comment

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

All these are required properties?

locale: PropTypes.string
}

static defaultProps = {
locale: 'en'
}

render() {
const {theme, username, password, remember, onUsernameChange, onPasswordChange, onRememberToggle, onLogin, locale} = this.props;
return (
<IntlProvider locale={locale} messages={getLocaleMessage(locale)}>
Copy link
Collaborator

Choose a reason for hiding this comment

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

I feel wrapping every component with localization wrapper feels like an overkill. Instead, can we do this once is App and use redux store to save the locale and use connect to listen to it

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree, putting this in Redux is a good solution because we can dynamically load it from the server.

<div className={theme.container}>
<fieldset>
<legend>
<FormattedMessage
id="LOGIN"
/>
</legend>
<div className={theme.property}>
<label
className={theme.propertyLabel}
htmlFor='uname'
>
<FormattedMessage
id="USERNAME"
/>
</label>
<Input
type='text'
id='uname'
placeholder={placeholderMessages.username}
required={true}
value={username}
onChange={(e) => { onUsernameChange(e.target.value); }}
/>
<Input
type='checkbox'
id='remember'
required={true}
value={remember}
onChange={(e) => { onRememberToggle(e.target.checked); }}
/>
<label
className={theme.propertyLabel}
htmlFor='remember'
>
<FormattedMessage
id="REMEMBER"
/>
</label>
</div>
<div className={theme.property}>
<label className={theme.propertyLabel} htmlFor='pword'>
<FormattedMessage
id="PASSWORD"
/>
</label>
<Input
type='password'
id='pword'
placeholder={placeholderMessages.password}
required={true}
value={password}
onChange={(e) => { onPasswordChange(e.target.value); }}
/>
</div>
<div className={theme.login}>
<button type='submit' onClick={() => { onLogin(username, password); }}>
<FormattedMessage
id="SUBMIT"
/>
</button>
</div>
</fieldset>
</div>
</IntlProvider>
);
}
}

const ReduxLogin = connect((state) => {
const {login} = state;
return {
username: login.username,
password: login.password,
remember: login.remember
};
}, (dispatch) => {
return {
onUsernameChange: (val) => {dispatch(setUsername(val)); },
onPasswordChange: (val) => {dispatch(setPassword(val)); },
onRememberToggle: (val) => {dispatch(toggleRemember(val)); },
onLogin: (uid, password) => {dispatch(attemptLogin(uid, password)); }
};
})(Login);

export {Login as LoginBase};
export default themr('LOGIN', style)(ReduxLogin);
11 changes: 11 additions & 0 deletions src/Login/l10n/en_US.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
export default {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we make these json files?

LOGIN: 'Login',
USERNAME: 'Username',
PASSWORD: 'Password',
REMEMBER: 'Remember',
SUBMIT: 'Submit',
FULLNAME: 'Full name',
REGISTER: 'Register',
'placeholder.username': 'Please input your username',
Copy link
Contributor

Choose a reason for hiding this comment

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

Not a fan of mixing ALLCAPS with dot.format.

'placeholder.password': 'Please input your password'
};
11 changes: 11 additions & 0 deletions src/Login/l10n/zh_CN.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
export default {
LOGIN: '登录',
USERNAME: '用户名',
PASSWORD: '密码',
REMEMBER: '记住我',
SUBMIT: '确认',
FULLNAME: '全名',
REGISTER: '注册',
'placeholder.username': '请输入用户名',
'placeholder.password': '请输入密码'
};
8 changes: 4 additions & 4 deletions test/login.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,18 +8,18 @@ describe('<Login />', () => {

it('should include username input', () => {
const wrapper = shallow(<LoginBase theme={theme}/>);
// console.log(wrapper.debug());
expect(wrapper.find('input[type="text"]')).to.have.length(1);
// console.log(wrapper.find('[type="text"]').debug());
expect(wrapper.find('[type="text"]')).to.have.length(1);
});

it('should include password input', () => {
const wrapper = shallow(<LoginBase theme={theme}/>);
expect(wrapper.find('input[type="password"]')).to.have.length(1);
expect(wrapper.find('[type="password"]')).to.have.length(1);
});

it('should include checkbox input', () => {
const wrapper = shallow(<LoginBase theme={theme}/>);
expect(wrapper.find('input[type="checkbox"]')).to.have.length(1);
expect(wrapper.find('[type="checkbox"]')).to.have.length(1);
});

it('should include submit button', () => {
Expand Down