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

Black and White listings for the Extension Manager #7989

Merged
merged 59 commits into from Mar 28, 2020

Conversation

echarles
Copy link
Contributor

@echarles echarles commented Mar 4, 2020

This is a PR to implement Black and White listings for the Extension Manager as discussed in #7933 and #7967.

It must be seen in conjunction with jupyterlab/jupyterlab_server#82

@jupyterlab-dev-mode
Copy link

@jupyterlab-dev-mode jupyterlab-dev-mode bot commented Mar 4, 2020

Thanks for making a pull request to JupyterLab!

To try out this branch on binder, follow this link: Binder

@github-actions github-actions bot added tag:Design System CSS pkg:extensionmanager pkg:ui-components tag:CSS tag:Examples labels Mar 4, 2020
Copy link
Member

@saulshanabrook saulshanabrook left a comment

Nice work!

One question I had was about where the blacklist would be served from. I assumed it would be fetched directly from a github repo, not served locally, so that any changed to it would happen without having to upgrade JupyterLab.

As far as I can tell, this PR takes a different approach of serving the file locally, is that right?

@echarles
Copy link
Contributor Author

@echarles echarles commented Mar 7, 2020

Thx @saulshanabrook for looking at that. I should have bring more context around the work done so far.

First, I have added a comment here further to the discussion at the jupyterlab meeting. The idea is to not allow the user to change the URIs via settings but rather to enforce them via configuration on server side.

Therefor, this PR does an initial fetch on the server to get the URIs and then fetches the effective listings from whatever defined (github repo...)

I have added a simple example in example/listings folder for dev/experiment purposes that exposes the listings as an additional handler on the jupyterlab_server.

But yes, the goal is to host those listings on github.

Hope this clarifies a bit. I still need to document all that but I will do it when it will be a bit more stable.

@echarles
Copy link
Contributor Author

@echarles echarles commented Mar 8, 2020

Current status.

demo

@saulshanabrook saulshanabrook added this to the 3.0 milestone Mar 20, 2020
@MarsBarLee
Copy link
Contributor

@MarsBarLee MarsBarLee commented Mar 25, 2020

I have a suggestion- we could change the checkbox to a red 'Enable' button and a blue 'Disable' button. My reasoning is that a red 'Enable' button acts more as a stronger cautionary warning than the checkbox. By creating more 'friction' in the user's experience, it requires more thought and puts more liability on the user's side, rather than JupyterLab's side.
Screen Shot 2020-03-25 at 10 59 36 AM

After enabling:
Screen Shot 2020-03-25 at 10 59 48 AM

@saulshanabrook
Copy link
Member

@saulshanabrook saulshanabrook commented Mar 25, 2020

@MarsBarLee I like how it looks better too!

@tgeorgeux
Copy link
Contributor

@tgeorgeux tgeorgeux commented Mar 25, 2020

@MarsBarLee That looks good. I don't have a strong opinion, but we also discussed grey for the disable button. I have a weak preference for blue over grey. In that once that warning is opened blue is the lowest friction 'disable this dangerous thing' option. I like it a lot better than the check box.

Mars and I did a UX Review I've asked her to post it below.

@MarsBarLee
Copy link
Contributor

@MarsBarLee MarsBarLee commented Mar 25, 2020

Hey Eric, I have my feedback here.

UX Review

For the warning text:
  • Change the font size to --jp-ui-font-size1.
  • Change font weight to regular.
  • Add a period at end of first paragraph.
  • Add a page break in-between paragraphs.(You could also add 8px padding between first and second paragraph if that's easier.)

For the UI, when the extension installer is disabled

  • The DISCOVER heading should be disabled and greyed out. Use --jp-layout-color3 for the disabled heading. (If it's too diffilcult to add the color change, at least close and disable interactions)
  • The INSTALLER header should be be disabled and collapsed as well. Use --jp-layout-color3 as well. (Same as above)
  • Add the disabled property to the refresh button

For the search bar at the top, change the icon to the same icon already used in the command palette search bar.

With these changes included we should be good to include in JupyterLab 2.1

@echarles
Copy link
Contributor Author

@echarles echarles commented Mar 27, 2020

@MarsBarLee Thx for the enhancements. I have implemented the following:

Warning text:

  • Change the font size to --jp-ui-font-size1
  • Change font weight to regular.
  • Add a period at end of first paragraph.
  • Add a page break in-between paragraphs

When the extension installer is disabled.

  • The DISCOVER and INSTALLER heading should be disabled and greyed out.
  • Add the disabled property to the refresh button

For the search bar at the top

  • change the icon to the same icon already used in the command palette search bar => The search widget in command palette is different from the one used in
    the extension manager. I have done CSS work to make them identical but at some point, I face issues which are not solvable unless I completely migrate one solution
    to the other solution. I have just changed the icon which even does not display at the same scale. The blue box in the command palette can not be removed easily
    (I guess it has been added on purpose...)

Here is what it gives.

Untitled

@saulshanabrook
Copy link
Member

@saulshanabrook saulshanabrook commented Mar 27, 2020

I am trying this locally without a blacklist or whitelist. It seems like there might be some error there in that logic? The server seems to returning no whitelist of blacklist URLs (as you can see from the network response here), but it is saying an extension is not on the whitelist.

Could you try to verify this locally to see if it works for you?

Screen Shot 2020-03-27 at 1 57 27 PM

@echarles
Copy link
Contributor Author

@echarles echarles commented Mar 27, 2020

Gosh, you saved the lab from complete extension breakdown..., Saul. I have pushed 66e4921 to fix that and also tested everything in the 3 modes (default, black, white).

@saulshanabrook saulshanabrook self-requested a review Mar 27, 2020
Copy link
Member

@saulshanabrook saulshanabrook left a comment

OK it works for me now! I will merge when tests pass.

@echarles
Copy link
Contributor Author

@echarles echarles commented Mar 28, 2020

@saulshanabrook Based on our conversation, I have pushed de5de53 to ensure that the extensions discovery tab is populated on first manager display (this is a behavior change compared to master, but as discussed it is a good enhancement).

CI is back green (except the notable Linux Docs), but we know that we can consider it Green as it is a know issue tracked in #8068.

LGTM to merge.

@saulshanabrook saulshanabrook modified the milestones: 3.0, 2.1 Mar 28, 2020
Copy link
Member

@saulshanabrook saulshanabrook left a comment

OK the default load now works. Just had one more fix, so that react doesn't give a console warning about keys.

packages/extensionmanager/src/widget.tsx Outdated Show resolved Hide resolved
Co-Authored-By: Saul Shanabrook <s.shanabrook@gmail.com>
@saulshanabrook saulshanabrook merged commit 8465708 into jupyterlab:master Mar 28, 2020
70 of 76 checks passed
@lock lock bot added the status:resolved-locked label May 6, 2020
@lock lock bot locked as resolved and limited conversation to collaborators May 6, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
documentation pkg:extensionmanager pkg:ui-components status:resolved-locked status:Work in Progress tag:CSS tag:Design System CSS tag:Examples
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants