Use input type password for login field

[k5o]

What current issue(s) from Trello/Github does this address?
#53

What problem does this PR solve?
Visually exposing private keys at login

How did you solve this problem?
Default to input type password instead of text for the login input. The input can be visible by clicking a toggle button.

How did you make sure your solution works?
Unit test, visual confirmation

Are there any special changes in the code that we should be aware of?
LoginPrivateKey is now a React.Component

Is there anything else we should know?
No

• Unit tests written?

[M-L-A]

A toggle for this may be useful- some people may want to visually check that their key is correct before attempting to login

[ghost]

I would also appreciate a toggle. Great enhancement though.

[k5o]

NOT CURRENTLY WORKING

@anfn101 since login authenticates in real-time on each onWifChange, the current flow expects the last user action at the login page to be an input change.

Unfortunately that means if the last user action is anything else (I added a onShowKeyClick handler), the user won't be able to login. As of right now the toggle works but I can't figure out an eloquent solution to this login action problem without heavy coupling. Would you mind taking a look? Thanks.

[golivera]

I don't think you need this showKey here. Probably was part of an earlier implementation.

[k5o]

Yes, my mistake, thanks for catching that

[golivera]

Do you also need to do {...state, showKey: action.showKey}; to allow the rest of the state to propagate down into the component?

edit:
Just to clarify in seems that on LOGOUT all the state is being modified so that's why there is no spread operator on the state. In this case we are modifying only one variable so we want the rest to trickle down. Otherwise, the Login component only receives the showKey state without any other state.

[k5o]

You're absolutely right, thanks!

[k5o]

Thanks to @golivera for figuring out the issue (I'm still new to redux). Please let me know if there's anything else to change/add.

[golivera]

I think testing was added recently so that could be something to check out.

[k5o]

@golivera Added the base test but I'd also like to test the inverse toggle after the initial toggle action has been sent (i.e. state has changed). I'm having trouble reflecting the state change though, could you help me with that?

[airoude]

This toggling state should be handled by the Login component itself for coherence. This state change isn't application wide necessary.

[k5o]

Thanks, I agree it's not an app-wide state, but I'm not entirely sure how to contain this state change to Login only, could you elaborate further? (New to redux but not react, apologies)

[airoude]

So to have component state you need to change it first from a function to a class that extends the React component class. Then we can have self contained states, like in the example below.

Feel free to use this but I didn't test it.

class Login extends Component {
    static propTypes = {
        loggedIn: PropTypes.bool.isRequired,
        wif: PropTypes.string.isRequired,
    };

    state = {
        showKey: false
    };

    toggleKeyVisibility = () => {
        this.setState(prevState => ({
            showKey: !prevState.showKey
        }));
    };
    
    handleInputChange = e => {
        this.setState({
            [e.target.name]: e.target.value
        });
    };

    render = () => {
        const { loggedIn, wif } = this.props;
        const { showKey } = this.state;

        return (
            <div id="loginPage">
                <div className="login">
                    <div className="logo">
                        <img src={logo} width="60px" />
                    </div>

                    <input type={showKey ? 'text' : 'password'}
                           placeholder="Enter your private key here (WIF)"
                           id="inputKey"
                           name="wif"
                           onChange={this.handleInputChange} />

                    {showKey ?
                        <FaEyeSlash className="viewKey" onClick={this.toggleKeyVisibility} /> :
                        <FaEye className="viewKey" onClick={this.toggleKeyVisibility} />}

                    <div className="loginButtons">
                        {loggedIn ?
                            <Link to="/dashboard">
                                <button>Login</button>
                            </Link> :
                            <button disabled="true">Login</button>}
                        <Link to="/create">
                            <button>New Wallet</button>
                        </Link>
                    </div>
                </div>
            </div>
        )
    }
}

const mapStateToProps = state => ({
    loggedIn: state.account.loggedIn,
    wif: state.account.wif,
});

const mapActionCreators = (dispatch) => {
    onWifChange: (dispatch, value) => {
        // TODO: changed back to only WIF login for now, getting weird errors with private key hex login
        dispatch(login(value));
    }
};

export default connect(mapStateToProps, mapActionCreators)(Login);

[k5o]

Great! In my new commit I switched it to a React.Component with some appropriate React-specific version cleanup and compatibility with Redux. I appreciate the help.

[Ejhfast]

thanks for this! @k5o if you could fix the conflicts (and see the convo in #177) would be happy to merge

[k5o]

@Ejhfast Sorry for delay, been busy. I just applied the key toggling to all the login views. Do I need to apply it to the encrypted key field on the Nep2 login as well?

[Ejhfast]

@k5o no, that field should be fine -- will take a look!

[Ejhfast]

@k5o this looks good! it seems that you are using react state to manage the toggle, though? i would like to keep state managed by redux, does that make sense? so we could have a toggle setting somewhere like here (https://github.com/CityOfZion/neon-wallet/blob/master/app/modules/metadata.js) and could reference through the props

[k5o]

@Ejhfast In my original implementation I managed the toggle via the reducer but @golivera suggested that this toggle shouldn't be an app-wide concern, but rather a concern of the view that contains it, which I thought was a good idea. See discussion: #69 (comment)

I can do it either way but I'd like to get confirmation first before proceeding, thanks

[shawnmclean]

I agree that this state should be encapsulated by the component. More reading from Dan Abramov: reduxjs/redux#1287