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

Dialog refactor #2661

Merged
merged 15 commits into from Jul 20, 2017
Merged

Dialog refactor #2661

merged 15 commits into from Jul 20, 2017

Conversation

@blink1073
Copy link
Member

@blink1073 blink1073 commented Jul 11, 2017

Refactored dialog to properly use widget lifecycle events. Extracts the value from the body widget before disposing of it. Also accepts virtual dom content for static dialogs.

h.br(),
'the command line and relaunch'
])
]);
Copy link
Contributor

@sccolbert sccolbert Jul 11, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The arrays are not needed. The h functions accept varargs.

@@ -866,6 +872,43 @@ namespace Private {
*/
export
function selectKernel(session: ClientSession): Promise<Kernel.IModel> {
let select = Dialog.okButton({ label: 'SELECT' });
Copy link
Contributor

@sccolbert sccolbert Jul 11, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this button a separate line, but not the cancel button?

/**
* Create a dialog panel instance.
*
* @param options - The dialog setup options.
*/
constructor(options: Dialog.IOptions={}) {
constructor(options: Dialog.IOptions<T>={}) {
super();
this.addClass('jp-Dialog');
options = Private.handleOptions(options);
Copy link
Contributor

@sccolbert sccolbert Jul 11, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This form of option handling is not null safe. The type of options is still an object with all optional properties.

let body = new RenameHandler(oldPath);
return showDialog({
title: 'Rename File',
body,
Copy link
Contributor

@sccolbert sccolbert Jul 11, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

body: new RenameHandler(oldPath)

'For more information, see the',
h.a({ href: 'http://ipython.org/ipython-doc/2/notebook/security.html' },
'Jupyter security documentation'),
]);
Copy link
Contributor

@sccolbert sccolbert Jul 11, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

array not needed.

buttons: [Dialog.cancelButton(), select]
}).then(result => {
if (!result.accept) {
return void 0;
Copy link
Contributor

@sccolbert sccolbert Jul 11, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the return type should be Promise<Kernel.IModel | null> and then you can just return result.value, or this undefined needs to be replaced with an actual model.


let layout = this.layout = new PanelLayout();
let content = new Panel();
content.addClass('jp-Dialog-content');
layout.addWidget(content);

this._body = options.body;

let header = renderer.createHeader(options.title);
let body = renderer.createBody(options.body);
let footer = renderer.createFooter(this._buttonNodes);
content.addWidget(header);
Copy link
Contributor

@sccolbert sccolbert Jul 11, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is there an extra content widget, which is the only child of the dialog? Why not just add the header, body, and footer directly to the layout?

Copy link
Member Author

@blink1073 blink1073 Jul 11, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The content is used to center the dialog on the host node with a faded background.

Copy link
Contributor

@sccolbert sccolbert Jul 11, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

got it.


this._primary = this._buttonNodes[this._defaultButton];

if (options.primaryElement) {
Copy link
Contributor

@sccolbert sccolbert Jul 11, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a fan of "primaryElement" name, when its actually a query string. Something more descriptive like "focusNodeSelector" or "focusQuerySelector" or "primaryElementSelector" would be better.

this.dispose();
promise.resolve({
button,
accept: button.accept,
Copy link
Contributor

@sccolbert sccolbert Jul 11, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this seems a bit redundant. If accept always comes from the button, why include it as a separate property?

super();
this.addClass('jp-Dialog');
options = Private.handleOptions(options);
options = Private.handleOptions(options || {});
Copy link
Contributor

@sccolbert sccolbert Jul 11, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why the || {}?

@blink1073
Copy link
Member Author

@blink1073 blink1073 commented Jul 20, 2017

Rebased and adding strict null checks.

@blink1073 blink1073 merged commit b073429 into jupyterlab:master Jul 20, 2017
2 checks passed
@blink1073 blink1073 mentioned this pull request Jul 21, 2017
@blink1073 blink1073 deleted the dialog-refactor branch Aug 7, 2017
@lock lock bot locked as resolved and limited conversation to collaborators Aug 9, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants