Skip to content
This repository has been archived by the owner on Apr 15, 2019. It is now read-only.

Add option to remember account for read-only access - Closes #559 #737

Merged
merged 33 commits into from
Sep 19, 2017

Conversation

slaweet
Copy link
Contributor

@slaweet slaweet commented Sep 12, 2017

Closes #559

@slaweet slaweet self-assigned this Sep 12, 2017
@slaweet slaweet changed the title Add option to remember account for read-only access after app was closed - Closes #559 Add option to remember account for read-only access - Closes #559 Sep 13, 2017
import styles from './account.css';

const getStatusTooltip = (props) => {
if (props.secondSignature) {
Copy link
Contributor

Choose a reason for hiding this comment

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

When the condition statement is long, sometimes the combination of ternary and if-else blocks helps keep the code readable, but in simple conditions please prefer a single type

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed. Good point

error={this.props.secondPassphrase.error}
value={this.props.secondPassphrase.value}
onChange={this.onChange.bind(this, 'secondPassphrase')} /> :
null)}
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here

error={this.props.passphrase.error}
value={this.props.passphrase.value}
onChange={this.onChange.bind(this, 'passphrase')} /> :
null)}
Copy link
Contributor

Choose a reason for hiding this comment

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

{ conditionStatement && <Component /> } equivalents and also helps eliminate the tailing null.

<TooltipIconButton className={`show-passphrase-toggle ${styles.eyeIcon}`}
tooltipPosition='horizontal'
tooltip={this.state.inputType === 'password' ? 'Show passphrase' : 'Hide passphrase'}
icon='remove_red_eye'
Copy link
Contributor

Choose a reason for hiding this comment

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

Please replace the icon name string value with:
{ this.state.inputType === 'password' ? 'visibility' : 'visibility_off' }


render() {
return (getSavedAccount() ?
<MenuItem caption="Forget this account locally"
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe Forget this account? since we don't use the words remove or delete, adding the adverb locally may create confusion. (This is just a suggestion)

className='forget-account'
onClick={this.removeSavedAccount.bind(this)}
/> :
<MenuItem caption="Save this account locally"
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the more common sentence for this concept is Remember this account.

export default class SaveAccountButton extends React.Component {
removeSavedAccount() {
removeSavedAccount();
this.props.successToast({ label: 'Account forgotten locally.' });
Copy link
Contributor

Choose a reason for hiding this comment

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

Successfully forgot this account?

reyraa
reyraa previously approved these changes Sep 19, 2017
Copy link
Contributor

@reyraa reyraa left a comment

Choose a reason for hiding this comment

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

Thanks Vit. Excellent!

@slaweet slaweet merged commit 92ed825 into 1.2.0 Sep 19, 2017
@slaweet slaweet deleted the 559-remeber-account branch September 19, 2017 10:36
@slaweet slaweet restored the 559-remeber-account branch September 19, 2017 10:39
@slaweet slaweet deleted the 559-remeber-account branch September 19, 2017 13:32
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants