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

Add placeholder attribute to text widgets #5652

Merged
merged 6 commits into from Apr 18, 2014

Conversation

Projects
None yet
3 participants
@jhamrick
Contributor

jhamrick commented Apr 17, 2014

This fixes #5642

Should I try to write a test for this? Can someone point me to a resource on writing tests for javascript?

@jhamrick

This comment has been minimized.

Show comment
Hide comment
@jhamrick

jhamrick Apr 17, 2014

Contributor

Updated this to work with text areas as well. I also updated the Widget Basics example notebook -- is this the appropriate place to mention placeholders, or should I move the example somewhere else?

Contributor

jhamrick commented Apr 17, 2014

Updated this to work with text areas as well. I also updated the Widget Basics example notebook -- is this the appropriate place to mention placeholders, or should I move the example somewhere else?

@jdfreder

This comment has been minimized.

Show comment
Hide comment
@jdfreder

jdfreder Apr 17, 2014

Member

Looks good to me. 👍

The HTML tests for the string widgets are in ipython/IPython/html/tests/widgets/widget_string.js and all of the utility functions (and base class) can be found in ipython/IPython/html/tests/util.js. Programming tests for the JS stuff is a little wonky because you have to deal with two execution contexts. I think @ivanov wrote a nice excerpt about this here.

I also updated the Widget Basics example notebook -- is this the appropriate place to mention placeholders, or should I move the example somewhere else?

Sounds good.

Member

jdfreder commented Apr 17, 2014

Looks good to me. 👍

The HTML tests for the string widgets are in ipython/IPython/html/tests/widgets/widget_string.js and all of the utility functions (and base class) can be found in ipython/IPython/html/tests/util.js. Programming tests for the JS stuff is a little wonky because you have to deal with two execution contexts. I think @ivanov wrote a nice excerpt about this here.

I also updated the Widget Basics example notebook -- is this the appropriate place to mention placeholders, or should I move the example somewhere else?

Sounds good.

@jhamrick

This comment has been minimized.

Show comment
Hide comment
@jhamrick

jhamrick Apr 17, 2014

Contributor

Cool, thanks, I'll look into writing a test then.

Contributor

jhamrick commented Apr 17, 2014

Cool, thanks, I'll look into writing a test then.

@jhamrick

This comment has been minimized.

Show comment
Hide comment
@jhamrick

jhamrick Apr 17, 2014

Contributor

Ok, wrote a javascript test for checking whether the placeholder is properly set.

Contributor

jhamrick commented Apr 17, 2014

Ok, wrote a javascript test for checking whether the placeholder is properly set.

@jdfreder jdfreder added this to the 3.0 milestone Apr 17, 2014

@jdfreder

This comment has been minimized.

Show comment
Hide comment
@jdfreder

jdfreder Apr 17, 2014

Member

Awesome, looks great! I'll pull it and test locally. Once Travis accepts it, I'll merge if no one objects.

Member

jdfreder commented Apr 17, 2014

Awesome, looks great! I'll pull it and test locally. Once Travis accepts it, I'll merge if no one objects.

@minrk

This comment has been minimized.

Show comment
Hide comment
@minrk

minrk Apr 18, 2014

Member

Looks good to me, go ahead and merge @jdfreder.

Member

minrk commented Apr 18, 2014

Looks good to me, go ahead and merge @jdfreder.

jdfreder added a commit that referenced this pull request Apr 18, 2014

Merge pull request #5652 from jhamrick/placeholder
Add placeholder attribute to text widgets

@jdfreder jdfreder merged commit 04f4cf1 into ipython:master Apr 18, 2014

1 check passed

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

This comment has been minimized.

Show comment
Hide comment
@jdfreder
Member

jdfreder commented Apr 18, 2014

Thanks @jhamrick

@jhamrick jhamrick deleted the jhamrick:placeholder branch Apr 19, 2014

mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this pull request Nov 3, 2014

Merge pull request #5652 from jhamrick/placeholder
Add placeholder attribute to text widgets
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment