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

Adding type='password' support for Text in TextView class. #1310

Merged
merged 8 commits into from Apr 27, 2017

Conversation

Projects
None yet
3 participants
@jakhani

jakhani commented Apr 25, 2017

Currently there is no way to use Text widget for Password. This code will allow to pass type while using Text widget which will allow to set type='password' for Text.

@jasongrout

This comment has been minimized.

Member

jasongrout commented Apr 25, 2017

Thanks! Up to now, we've made different types of html widgets different things (e.g., the checkbox, color, radio, etc.). Following that pattern, we'd have a Password() widget. What do you think about making a separate Password() widget that inherits from the text widget? On the other hand, I can see this just being basically a small visual tweak on a textbox, so perhaps making a password=True/False attribute on a textbox would work well.

@jakhani

This comment has been minimized.

jakhani commented Apr 25, 2017

@jasongrout This is to follow the way html works. In HTML, when you pass type="password", it doesn't show it's value. If there is any other way already available, can you please share some sample code with me?

@jasongrout

This comment has been minimized.

Member

jasongrout commented Apr 25, 2017

@jakhani - I agree it would be great to make this available to users, and there's no way to do it right now. What I'm suggesting is that we either (a) make a new Password() that acts almost exactly like a text widget, except that in the view we set the type to password, or (b) make a single password attribute to the Text() widget that controls this switch. Each of those options makes it easy to expose this one thing (a password box) in a pythonic api.

@jasongrout

This comment has been minimized.

Member

jasongrout commented Apr 25, 2017

(while exposing the input type exposes all of the sorts of inputs HTML has to offer: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input)

@jasongrout jasongrout added this to the 7.0 milestone Apr 25, 2017

@jakhani

This comment has been minimized.

jakhani commented Apr 26, 2017

@jasongrout Sure. I have added new widget Password. I have tested it and it's working fine. Can you please review this code?

@jasongrout

Thanks for doing a good comprehensive job here. Looking over the code, it may be much easier (and much less code) to inherit from the Text widget and just tweak the necessary parts.

@register
class Password(_String):

This comment has been minimized.

@jasongrout

jasongrout Apr 26, 2017

Member

Would it be easier to just inherit from Text?

@register
class Password(Text):
    _view_name = ...
    _model_name = ...

Then you inherit all of the text behavior automatically.

}
export
class PasswordView extends LabeledDOMWidgetView {

This comment has been minimized.

@jasongrout

jasongrout Apr 26, 2017

Member

Again, here how about just inheriting from TextView, and doing something like:

render() {
    super.render();
    this.textbox.setAttribute('type', 'password');
}

and let everything else be inherited?

This comment has been minimized.

@jasongrout

jasongrout Apr 26, 2017

Member

(the only issue I could see here is if the element is appended to the DOM before the input is set to password, the default string might flash up on the screen exposed)

This comment has been minimized.

@jasongrout

jasongrout Apr 26, 2017

Member

(i.e., that's something we should check to see if it happens)

This comment has been minimized.

@jasongrout

jasongrout Apr 26, 2017

Member

In fact, why don't we change TextView to have a new protected attribute (at the bottom of the class), the inputType, which for TextView is 'text' and is used in the render() method, but the PasswordView subclass just sets this to 'password'. Then the PasswordView just becomes something like:

export
class PasswordView extends TextView {
    protected inputType = 'password';
}

or something like that.

@jakhani

This comment has been minimized.

jakhani commented Apr 27, 2017

@jasongrout I have incorporated all your comments and submitted changes. Can you please have a look at it?

jasongrout added some commits Apr 27, 2017

@@ -661,6 +661,23 @@ Attribute | Type | Default | Help
`placeholder` | string | `'\u200b'` | Placeholder text to display when nothing has been typed
`value` | string | `''` | String value
### Jupyter.Password

This comment has been minimized.

@jasongrout

jasongrout Apr 27, 2017

Member

Can you delete the changes to this file? This file is documenting the widgets as of 2.1.0 and is automatically generated. We'll generate another listing when we release the next version.

@jasongrout

This comment has been minimized.

Member

jasongrout commented Apr 27, 2017

@jasongrout I have incorporated all your comments and submitted changes. Can you please have a look at it?

I made a few small changes (reformatting, and changing the inputType to also be readonly), and left one more comment. I'll test this tomorrow, but the code looks good to me now. Feel free to squash in my changes if you want, or leave them as separate commits - either way is fine.

Thanks again!

@jakhani

This comment has been minimized.

jakhani commented Apr 27, 2017

I have removed schema file changes. One check is failing and I am not able to get logs for the same. Can you please share detail on that in case you find anything?

Text inputType needs to be a string
If we don't broaden the type to `string`, then overriding with a different literal throws a type error.
@jasongrout

This comment has been minimized.

Member

jasongrout commented Apr 27, 2017

It seems like there is some problem with the logs on Travis right now. Hopefully my fix fixes whatever the error was.

@jasongrout jasongrout merged commit c3e7ee6 into jupyter-widgets:master Apr 27, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jasongrout

This comment has been minimized.

Member

jasongrout commented Apr 27, 2017

Thank you very much!

@jasongrout jasongrout referenced this pull request Apr 27, 2017

Closed

Update Changelog #1279

@jakhani

This comment has been minimized.

jakhani commented Apr 27, 2017

@jasongrout Thanks for helping and merging it quickly.

@jasongrout

This comment has been minimized.

Member

jasongrout commented Apr 27, 2017

And congratulations on your first contribution to ipywidgets!

@maartenbreddels

This comment has been minimized.

Member

maartenbreddels commented Apr 27, 2017

Indeed, thanks for this contribution! 👍

@pranayhasan pranayhasan referenced this pull request May 1, 2017

Closed

Add Kerberos Support #282

@jakhani

This comment has been minimized.

jakhani commented May 5, 2017

@jasongrout & @maartenbreddels I am facing issues while releasing new version. We need this Password widget changes released in new version. Can you please help me out by releasing new version of ipywidgets?

@jasongrout

This comment has been minimized.

Member

jasongrout commented May 5, 2017

I thought this was in the alpha version - can you use our alpha release?

We are working hard on finishing up what is needed for the 7.0 final release. If you need this widget before 7.0 is released, you can make a new python package with just this widget and use that in the meantime, right?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment