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

Displays Warning Message if Cookies Not Enabled #3511

Merged
merged 13 commits into from Apr 10, 2018

Conversation

Projects
None yet
3 participants
@ckilcrease
Copy link
Contributor

ckilcrease commented Apr 6, 2018

This addresses #2337

Adds a check for cookies using navigator.cookiesEnabled (tested in Chrome 64.0.3282.186, works both when cookies are blocked altogether and when only blocked for localhost:8888). If the user doesn't have cookies enabled, a request to enable cookies appears in body of modal as follows:

screen shot 2018-04-06 at 12 34 51 pm

rosaswaby and others added some commits Apr 5, 2018

@takluyver

This comment has been minimized.

Copy link
Member

takluyver commented Apr 7, 2018

Thanks, this is looking good. A couple of things:

  • Could we ask you to do this with your editor configured not to strip trailing spaces? If you look at the changes, you'll see that >100 lines are changed because of automatically removed whitespace. This can cause unnecessary merge conflicts, and it makes git blame less useful.
  • Having two buttons seems redundant; the user doesn't really have two options. Maybe get rid of the 'OK' button in this case?
  • I think it would be good for the message to mention that Jupyter needs cookies to work. Also, if we can find a good website explaining what cookies are and how to make sure they're enabled on different browsers, we could link to it.
@ckilcrease

This comment has been minimized.

Copy link
Contributor Author

ckilcrease commented Apr 7, 2018

Definitely - we will get started on those changes! 👍

@ckilcrease ckilcrease closed this Apr 8, 2018

@ckilcrease

This comment has been minimized.

Copy link
Contributor Author

ckilcrease commented Apr 8, 2018

Made most of the changes discussed, but I wanted to check in re: websites on cookies before choosing one; I think MDN does a good job of explaining them concisely, although parts of the page go into details that might not really concern users (such as creating cookies, scope, etc.) - if this would be an issue, I can find one that only presents the overview/relevant details. (Also, accidentally clicked "close and comment" earlier - apologies for that!)

@ckilcrease ckilcrease reopened this Apr 8, 2018

@takluyver

This comment has been minimized.

Copy link
Member

takluyver commented Apr 8, 2018

Thanks!

I like MDN, but I think it's too technical for this. If people want help enabling cookies, they're probably going to be put off by all the detail. I was thinking more of pages like this one or this one. (Those are just the first ones I found with a quick search; there may well be better ones out there).

@ckilcrease

This comment has been minimized.

Copy link
Contributor Author

ckilcrease commented Apr 10, 2018

That makes sense - I looked at a few other articles as well, but it seems like the WikiHow one is the most accurate (many seem to be outdated) and easy to follow, so at the moment I have that one linked as follows:

screen shot 2018-04-10 at 11 45 31 am

ckilcrease added some commits Apr 10, 2018

@takluyver takluyver added this to the 5.5 milestone Apr 10, 2018

@takluyver

This comment has been minimized.

Copy link
Member

takluyver commented Apr 10, 2018

Thanks @ckilcrease and @rosaswaby :-)

@takluyver takluyver merged commit ca3e7a3 into jupyter:master Apr 10, 2018

4 checks passed

codecov/patch Coverage not affected when comparing e3ee807...9bc3f99
Details
codecov/project 75.01% remains the same compared to e3ee807
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.