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

Extension disclaimers #4925

Merged
merged 11 commits into from Jul 19, 2018
Merged

Conversation

@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented Jul 19, 2018

Fixes #4820.

  1. Adds a badge to packages published under the @jupyterlab or @jupyter-widgets orgs.
  2. Sorts packages from those orgs to the top of the list.
  3. Gives a warning when you enable the extension manager that there are potential security risks.
  4. Makes the styling of the extension manager a bit more consistent with the rest of the app.

Before:
image

After:
image

Dialog:
image

A few questions:

  1. Am I missing any obvious npm orgs?
  2. Is this too heavy-handed in promoting @jupyterlab org extensions?
  3. How do we like the warning dialog language?
@ian-r-rose ian-r-rose added this to the Beta 3 milestone Jul 19, 2018
'However, we cannot vouch for every extension, ' +
'and some may introduce security risks. ' +
'Do you want to continue?',
buttons: [Dialog.cancelButton(), Dialog.okButton()]
Copy link
Member

@blink1073 blink1073 Jul 19, 2018

Choose a reason for hiding this comment

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

I think this should be a warnButton

@blink1073
Copy link
Member

@blink1073 blink1073 commented Jul 19, 2018

I'm fine with all three as is

Copy link
Member

@blink1073 blink1073 left a comment

LGTM, thanks! I'll let @jasongrout push the green button.

@vidartf
Copy link
Member

@vidartf vidartf commented Jul 19, 2018

What was the rationale for removing the warning colorbar? I'm not against it, just curious.

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Jul 19, 2018

What does the warning colorbar signify?

return showDialog({
title: 'Enable Extension Manager?',
body:
"Thanks for trying out JupyterLab's extension manager. " +
Copy link
Contributor

@jasongrout jasongrout Jul 19, 2018

Choose a reason for hiding this comment

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

We can use a backtick quote here and nicely format it, since the spacing is ignored in html, right?

Copy link
Member Author

@ian-r-rose ian-r-rose Jul 19, 2018

Choose a reason for hiding this comment

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

Can you explain more what you mean? As far as I know, leading and trailing spacing is ignored, but there is none of that here.

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Jul 19, 2018

What does the warning colorbar signify?

Ah, I see. It indicates the status from the server call: https://github.com/jupyterlab/jupyterlab/blob/master/jupyterlab/extension_manager_handler.py#L92

@ian-r-rose
Copy link
Member Author

@ian-r-rose ian-r-rose commented Jul 19, 2018

My rationale for removing the warning bar (summarized in 52770eb) was something like:

  1. It is not very clear that it indicates a warning, nor does it give much indication of what that warning is. Indeed, I didn't know it was a warning bar until getting into the code base. I thought it was something like an "installed" indicator.
  2. Relatedly, many extension installs produce some kind of warning. Some of those we should probably fix, but in practice the warning bar wasn't communicating much information to me.
  3. There can also be warnings associated with the overall build, such as WARNING in asset size limit: The following asset(s) exceed the recommended size limit (244 KiB). Again, we may want to fix that, but it doesn't seem fair to assign it to an extension.
  4. If a build produces a warning, it seems like it can come from any of the installed extensions (not just the one that you are installing at the moment). Actually associating the warning with a given extension seems fraught.

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Jul 19, 2018

Can we make the two confirmation dialog buttons "Enable" and "Disable"? I think best practices is to make the buttons verbs, to make it very clear what is going to happen if you press it.

@ian-r-rose
Copy link
Member Author

@ian-r-rose ian-r-rose commented Jul 19, 2018

Sure, I have relabeled the buttons.

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Jul 19, 2018

I haven't thoroughly reviewed the code, but I played with the end result, and looked at the code a bit. I'm +1 on merging, and hammering out other issues on another PR.

There are two other points in #4820 not addressed here: see #4820 (comment) for the discussion of those points.

@ian-r-rose
Copy link
Member Author

@ian-r-rose ian-r-rose commented Jul 19, 2018

I can take a look at the comment string here. I was under the impression that we decided not to add a permanent warning to the extension manager UI.

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Jul 19, 2018

I was under the impression that we decided not to add a permanent warning to the extension manager UI.

Given the Jupyter branding changes, I think I'm okay with that.

@blink1073
Copy link
Member

@blink1073 blink1073 commented Jul 19, 2018

Looks great, thanks!

@blink1073 blink1073 merged commit f154064 into jupyterlab:master Jul 19, 2018
1 check passed
@ian-r-rose ian-r-rose mentioned this pull request Jul 20, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Aug 8, 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.

4 participants