show inline reveal password option for auth

[tharun208]

insomnia.mov

Added reveal password ( eyeball icon) next to the password field to toggle visibility.

Hide reveal password ( eyeball icon) when a user opts to use show passwords from settings.

Came with a temporary improvement for this by #2970. I Will close that when this pr merges.

closes #2929

[apisaint]

This is awesome, what do you think about a single "OneLinePassword" (Input/Show-Hide) component? Seems like it would clean up the state on the parent as well as it's all contained within the component.

[tharun208]

This is awesome, what do you think about a single "OneLinePassword" (Input/Show-Hide) component? Seems like it would clean up the state on the parent as well as it's all contained within the component.

@apisaint, Yes we can and but we need the state showPassword which is used by TextInput. What I thought is to keep the state (map of authType with bool) in the auth-wrapper.js and pass it.

[apisaint]

This is awesome, what do you think about a single "OneLinePassword" (Input/Show-Hide) component? Seems like it would clean up the state on the parent as well as it's all contained within the component.

@apisaint, Yes we can and but we need the state showPassword which is used by TextInput. What I thought is to keep the state (map of authType with bool) in the auth-wrapper.js and pass it.

Absolutely, something like this? Note, I changed showPasswords to showAllPasswords but maybe it should be something like preferencesShowPasswords to indicate where it's from...

@autobind
class PasswordInput extends React.PureComponent<Props, State> {
  constructor(props: Props) {
    super(props);
    this.state = {
      showPassword: false,
    };
  }

  _handleShowPassword() {
    this.setState({ showPassword: !this.state.showPassword });
  }

  render() {
    const { showPassword } = this.state;
    const { showAllPasswords } = this.props;

    // Duplicate props for the OneLineEditor input, and remove additonal props
    // This way we can treat the element as a OneLineEditor component
    const inputProps = {...this.props};
    delete inputProps.showAllPasswords;

    const icon = {showPassword ? <i className="fa fa-eye-slash" /> : <i className="fa fa-eye" />}

    return (
      <>
        <OneLineEditor
          type={showAllPasswords ? 'text' : 'password'}
          type={showAllPasswords || showPassword ? 'text' : 'password'}
          id="password"
          {...inputProps}
        />
        </div>
        {!showAllPasswords ? (
        <Button
          className="btn btn--super-duper-compact pointer"
          onClick={this._handleShowPassword}
          value={showPassword}>
          {icon}
        </Button>
        ) : (<></>)}
      </>
    );
  }
}

Hopefully the project moves towards leveraging redux for preferences so that way you don't have to pass them to each component.

[tharun208]

This is awesome, what do you think about a single "OneLinePassword" (Input/Show-Hide) component? Seems like it would clean up the state on the parent as well as it's all contained within the component.

@apisaint, Yes we can and but we need the state showPassword which is used by TextInput. What I thought is to keep the state (map of authType with bool) in the auth-wrapper.js and pass it.

Absolutely, something like this? Note, I changed showPasswords to showAllPasswords but maybe it should be something like preferencesShowPasswords to indicate where it's from...

@autobind
class PasswordInput extends React.PureComponent<Props, State> {
  constructor(props: Props) {
    super(props);
    this.state = {
      showPassword: false,
    };
  }

  _handleShowPassword() {
    this.setState({ showPassword: !this.state.showPassword });
  }

  render() {
    const { showPassword } = this.state;
    const { showAllPasswords } = this.props;

    // Duplicate props for the OneLineEditor input, and remove additonal props
    // This way we can treat the element as a OneLineEditor component
    const inputProps = {...this.props};
    delete inputProps.showAllPasswords;

    const icon = {showPassword ? <i className="fa fa-eye-slash" /> : <i className="fa fa-eye" />}

    return (
      <>
        <OneLineEditor
          type={showAllPasswords ? 'text' : 'password'}
          type={showAllPasswords || showPassword ? 'text' : 'password'}
          id="password"
          {...inputProps}
        />
        </div>
        {!showAllPasswords ? (
        <Button
          className="btn btn--super-duper-compact pointer"
          onClick={this._handleShowPassword}
          value={showPassword}>
          {icon}
        </Button>
        ) : (<></>)}
      </>
    );
  }
}

Hopefully the project moves towards leveraging redux for preferences so that way you don't have to pass them to each component.

so, for now, can we live with this, and once support for redux is started, can we do the refactoring changes?

[develohpanda]

Hi @tharun208! Thanks for the PR. This generally looks good, although I agree with the previous review in that we can certainly avoid some of the duplication here but creating a PasswordEditor component, which wraps the OneLineEditor and the fa-eye-lash button.

Additionally, redux can already be used for this purpose. You can connect the PasswordEditor component, and use an existing selector to get the settings, without needing to pass it down through props from auth-wrapper.js. Redux is not used extensively within the project, but data from the database does exist within it and can be used in this manner.

// password-editor.js

const PasswordEditor = ({showPasswords, ...props}) = <>...</>;

const mapStateToProps = (state, props) => {
  const settings = selectSettings(state, props); // import from app/ui/redux/selectors.js

  return { showPasswords: settings.showPasswords };
};

export default connect(mapStateToProps)(PasswordEditor);

[tharun208]

Hi @tharun208! Thanks for the PR. This generally looks good, although I agree with the previous review in that we can certainly avoid some of the duplication here but creating a PasswordEditor component, which wraps the OneLineEditor and the fa-eye-lash button.

Additionally, redux can already be used for this purpose. You can connect the PasswordEditor component, and use an existing selector to get the settings, without needing to pass it down through props from auth-wrapper.js. Redux is not used extensively within the project, but data from the database does exist within it and can be used in this manner.

// password-editor.js

const PasswordEditor = ({showPasswords, ...props}) = <>...</>;

const mapStateToProps = (state, props) => {
  const settings = selectSettings(state, props); // import from app/ui/redux/selectors.js

  return { showPasswords: settings.showPasswords };
};

export default connect(mapStateToProps)(PasswordEditor);

@develohpanda sure will do this 👍🏻

[tharun208]

@develohpanda, I am done with the changes

[develohpanda]

Thanks for the updates and apologies for the delay. Well done for exploring the unit tests 👏🏽 I've just got a couple of observations from the updates.

Additionally, I've noticed there's a UI glitch introduced by these changes to the OneLineEditor.

Notice how the text shifts up and down on hovering over the element, however it doesn't happen in the live application right now. Is this something on my computer specifically or are you seeing the same?

[develohpanda]

prop-types is not really used for new components in this project (we'll look to remove it once we move to TS), so could you please translate this into a flow type? Have a look at many of the other methods. Don't worry about the exact types for the callback functions - just set them to be Function is they're not easy to find.

While the unit tests for this component all pass, they unfortunately litter the console with warnings due to prop-types. Changing to flow types should eliminate the warnings, and moving to TS will cause these errors at compile time instead of runtime.

[develohpanda]

Instead of accepting request as a prop, I feel that this generic component should accept password as a prop, and the onChange callback takes the updated password text as a parameter.

The calling component, in this case BasicAuth or DigestAuth, should contain the actual _handlePasswordChange implementation (which ultimately updates the database).

Essentially, I'm suggesting to separate the generic PasswordEditor component from the Request data model.

[tharun208]

@develohpanda, I addressed the feedbacks

[netlify]

Deploy preview for insomnia-storybook ready!

Built with commit 6e933b4

https://deploy-preview-2974--insomnia-storybook.netlify.app

[develohpanda]

One last item, please 🙂

[develohpanda]

Nice 👍

[dimitropoulos]

since this needs to be rebased anyway I thought I might ask if it's a simple change to extend the bottom line of the input to the eyeball so the eyeball is inline, e.g.:

compare that to what this PR implements.

but this is totally totally not blocking - feel free to disregard.