Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork3.8k
Split caption for pausing on exceptions#11970
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Thanks for making a pull request to jupyterlab! |
jasongrout commentedFeb 1, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I thought our convention was that the toggle status indicated state and the language didn't change. It's confusing for me to see a command checked and the language indicating the negative of the status. Edit: In other words, it's confusing to me to see |
krassowski commentedFeb 1, 2022
I think that we are looking at a different part of the UI@jasongrout. My comment was about the tooltip which currently shows But it seems you were thinking about title in the command palette, which already uses the format which you say is undesirable: I agree that for command palette the current state (i.e prior to this PR) is suboptimal. I think it should be showing "Pause on exceptions" with a tick or without a tick. It would be also useful to use the consistent capitalisation here (also the pause icon needs the css highlight class for contrast or to be removed). As for the button we should re-consider it's style also because while in 3.3 the background is grey and active button is white, in 4.0 it is reversed (the background is white and active button is grey) which will lead to confusion on future migration. 3.3 vs 4.0 changes apart, it is hard to know if the button is enabled or waiting to be pressed. Changing tooltip as in this PR will help a little (and possibly improve accessibility), but we can improve it a bit more I believe:
Let's see what others do:
|
Update wording to use the one seen in web browsers and VS Code
fcollonval commentedFeb 2, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Thanks for the research@krassowski Ind4860d1 I updated the wording to |
krassowski left a comment
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.
Thank you!
| ?trans.__('Disable pausing on exceptions') | ||
| :trans.__('Enable pausing on exceptions'), | ||
| caption:trans.__('Enable / Disable pausing on exceptions'), | ||
| label:trans.__('Pause on exceptions'), |
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.
For consistency with other items in palette we may prefer:
| label:trans.__('Pause onexceptions'), | |
| label:trans.__('Pause onExceptions'), |
but this is not a strong opinion.







References
#11752 (comment)
Code changes
Split the caption to highlight the status of the toggleable command
use function for label and caption so it gets properly updated when the command state changes
User-facing changes
Caption is clarified on the command status
Backwards-incompatible changes
N/A