Skip to content
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

Factor out output_prompt_function, as is done with input prompt #2774

Merged
merged 2 commits into from Sep 15, 2017

Conversation

Projects
None yet
3 participants
@iamlemec
Copy link
Contributor

iamlemec commented Aug 17, 2017

This allows for much cleaner customization of the output prompt in custom.js, as is currently the case with the input prompt.

@takluyver takluyver added this to the 5.2 milestone Aug 18, 2017

@takluyver

This comment has been minimized.

Copy link
Member

takluyver commented Aug 18, 2017

Thanks!

We're just trying to make a 5.1 release - I've marked this as 5.2 as it's probably not urgent enough to go in at the last minute. Hopefully we can merge it soon after the release.

@@ -467,6 +467,11 @@ define([
this.element.trigger('changed', {output_area: this});
};

OutputArea.output_prompt_classical = function(prompt_value) {
return '<bdi>'+i18n.msg.sprintf(i18n.msg._('Out[%d]:'),prompt_value)+'</bdi>';

This comment has been minimized.

@takluyver

takluyver Sep 15, 2017

Member

Can this construct a node like the old code does, rather than assembling an HTML string? While I don't think there's a security concern here, it's good practice to use the safer style throughout.

This comment has been minimized.

@iamlemec

iamlemec Sep 15, 2017

Author Contributor

Ok, just made the change. Btw, the code for input_prompt_classical in codecell.js also uses an HTML string, so it might be worth changing that too.

@iamlemec iamlemec force-pushed the iamlemec:master branch from 93aeaff to c946374 Sep 15, 2017

@gnestor

This comment has been minimized.

Copy link
Contributor

gnestor commented Sep 15, 2017

@iamlemec This looks good to me. I just re-ran the Travis build and it's passing now. @takluyver Is this ready to merge?

@takluyver

This comment has been minimized.

Copy link
Member

takluyver commented Sep 15, 2017

Yup, I think so.

@iamlemec thanks, and thanks for checking the input prompt method. If you want to make a separate PR to update that to create an element, I think we could merge that.

@takluyver takluyver merged commit 9dabaa2 into jupyter:master Sep 15, 2017

4 checks passed

codecov/patch Coverage not affected when comparing 5a0ee98...c946374
Details
codecov/project 79.36% remains the same compared to 5a0ee98
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.