-
Notifications
You must be signed in to change notification settings - Fork 294
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
update trusted icons and make trusted icon appear on the editor title #4430
Conversation
make trusted icon appear on the editor title
Codecov Report
@@ Coverage Diff @@
## main #4430 +/- ##
=====================================
Coverage 75% 75%
=====================================
Files 396 396
Lines 25861 25862 +1
Branches 3710 3707 -3
=====================================
+ Hits 19521 19577 +56
+ Misses 4813 4771 -42
+ Partials 1527 1514 -13
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code is fine just wanted to discuss quick that I think this is going to be an accessibility violation. This command is going to show up as a clickable button to a screen reader. And it's going to have no effect when run. Are we ok with that?
Claudia's ruling: have the notification pop on saying its already trusted. And the not trusted icon should be red |
the notebook is already trusted change untrusted icon to red remove command from the command palette
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would like to get @claudiaregio input on this.
I do'nt thinnk the button should be clickable to display messages that do absolutely nothing.
@@ -41,6 +41,13 @@ export class TrustCommandHandler implements IExtensionSingleActivationService { | |||
const context = new ContextKey('jupyter.trustfeatureenabled', this.commandManager); | |||
context.set(true).ignoreErrors(); | |||
this.disposables.push(this.commandManager.registerCommand(Commands.TrustNotebook, this.onTrustNotebook, this)); | |||
this.disposables.push( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should get this reviewed by a PM.
Generally I ton'd think its a good idea to display such messages. One could argue if the title indicates its trusted, then wy allow clickcing the button just to always (100% of time) display a message.
If the message is required, then i'd argue the meaning of the icon is not obvious to users.
@claudiaregio /cc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@DonJayamanne David talked to Claudia per his comment above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Claudia's ruling: have the notification pop on saying its already trusted.
"This notebook is already trusted."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Claudia just pinged me & stated that we need an icon that cannot be clicked.
I.e. just disable it and display tooltip.
She was under the impression we cannot disable it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Claudia just pinged me & stated that we need an icon that cannot be clicked.
I.e. just disable it and display tooltip.
She was under the impression we cannot disable it
For #4338, #4339
Here's what the new trusted icon looks like (tooltip says 'Trusted' and clicking does nothing):
And here's the new untrusted icon (brings up the trust notification):
package-lock.json
has been regenerated by runningnpm install
(if dependencies have changed).