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

Display "trusted" and "untrusted" notifications #1658

Merged
merged 2 commits into from Aug 5, 2016

Conversation

@gnestor
Copy link
Contributor

@gnestor gnestor commented Aug 4, 2016

demo

@gnestor
Copy link
Contributor Author

@gnestor gnestor commented Aug 4, 2016

Related #1293

@gnestor gnestor added this to the 4.3 milestone Aug 4, 2016
@gnestor
Copy link
Contributor Author

@gnestor gnestor commented Aug 4, 2016

@ellisonbg Can you UX test?

@minrk
Copy link
Member

@minrk minrk commented Aug 5, 2016

Nice!

@Carreau
Copy link
Member

@Carreau Carreau commented Aug 5, 2016

I would just not use the danger alternative as it's ok to view an untrusted notebook.
I think the "classic" color for both trusted and untrusted might be sufficient.

One of the reasons I think it would be better, is that I don't want this action to become something which is click-through like the UAC on windows.

Otherwise +1

@takluyver
Copy link
Member

@takluyver takluyver commented Aug 5, 2016

Yeah, we want to avoid creating something like the MS Office 'enable macros' dialog. Because when the computer says "This might be incredibly dangerous!", users say:

70494602

@gnestor
Copy link
Contributor Author

@gnestor gnestor commented Aug 5, 2016

Ok, I just switched to classic style.

One other question: Should the "Trusted" notification hide itself after a second or so? Currently both notifications are persisted. If the "Untrusted" notification is clicked, the user is prompted to trust the notebook. If the "Trusted" notification is clicked, the notification hides.

@Carreau
Copy link
Member

@Carreau Carreau commented Aug 5, 2016

Ok, I just switched to classic style.

One other question: Should the "Trusted" notification hide itself after a second or so? Currently both notifications are persisted. If the "Untrusted" notification is clicked, the user is prompted to trust the notebook. If the "Trusted" notification is clicked, the notification hides.

I think that's a good compromise. And it's easy enough to change afterward if needed.
It will keep the UI clean for most users.

As @Phyks was the first initiator of that, we can ask him his thoughts.

@takluyver
Copy link
Member

@takluyver takluyver commented Aug 5, 2016

I think I'd be inclined to hide both notifications after 10 seconds or so, to reduce the likelihood that the user will click to make it go away. Even in blue, it still stands out quite a bit.

@Carreau
Copy link
Member

@Carreau Carreau commented Aug 5, 2016

Even in blue, it still stands out quite a bit.

Both are gray now. IIUC.

@Phyks
Copy link

@Phyks Phyks commented Aug 5, 2016

Sorry, I did not have much time to follow it and eventually work on my PR lately.

Thanks @gnestor ! Your design implementation looks really cool!

I would not be in favor of hiding the untrusted notification, as the user may not notice it before it goes away and the main purpose of this feature was to have an indication that a notebook is untrusted. The trusted notification can go away though I think, as it results from either a user interaction, or a notebook being safe from the loading.

@gnestor
Copy link
Contributor Author

@gnestor gnestor commented Aug 5, 2016

Ok, @minrk should we switch this to 5.0 before merging or leave it at 4.3 and backport?

@Carreau
Copy link
Member

@Carreau Carreau commented Aug 5, 2016

Ok, @minrk should we switch this to 5.0 before merging or leave it at 4.3 and backport?
Show all checks

You can leave at 4.3 for the backport script to find the issue.
Or you can decide this is only 5.0 and is a "new feature" and leave switch the milestone to 5.0;
Up to you. I'm fine considering that as "new feature" and having one less thing to backport.

@gnestor
Copy link
Contributor Author

@gnestor gnestor commented Aug 5, 2016

I'm fine considering that as "new feature" and having one less thing to backport.

That's what I was thinking...

@gnestor gnestor modified the milestones: 5.0, 4.3 Aug 5, 2016
@gnestor gnestor merged commit 5a03481 into jupyter:master Aug 5, 2016
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@gnestor gnestor deleted the gnestor:trusted-notification branch Aug 5, 2016
@Carreau
Copy link
Member

@Carreau Carreau commented Aug 8, 2016

Yeah !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
5.0
Merged PRs
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.