Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

Closed
fcollonval wants to merge3 commits intomasterfromfcollonval-patch-1

Conversation

@fcollonval
Copy link
Member

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

@fcollonvalfcollonval added this to the3.3.x milestoneFeb 1, 2022
@jupyterlab-probot
Copy link

Thanks for making a pull request to jupyterlab!
To try out this branch onbinder, follow this link:Binder

@jasongrout
Copy link
Contributor

jasongrout commentedFeb 1, 2022
edited
Loading

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✓ Disable pausing on exceptions because it looks like the current state is to disable pausing on exceptions, rather than that is what the command will do. This contrasts with lots of other places where we use the toggle state to reflect the status of the statement, such as various "View" menu items.

@krassowski
Copy link
Member

I think that we are looking at a different part of the UI@jasongrout. My comment was about the tooltip which currently showsEnable / Disable pausing on exceptions:

enable-disable-button

But it seems you were thinking about title in the command palette, which already uses the format which you say is undesirable:

Screenshot from 2022-02-01 21-36-33
Screenshot from 2022-02-01 21-36-47

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:

Side note: regarding the enable/disable confusion, there is some more ideas on UX stackexchangehere andhere. One suggestion is to avoid buttons for what could be checkboxes.

Let's see what others do:

  • Chrome/Edge has tooltips for two states with tooltip "Pause on exceptions" when disabled (indicates action) and "Don't pause on exceptions" when enabled; when action is enabled, an extra checkbox "Pause on caught exceptions" shows up and the button is blue
    Screenshot from 2022-02-01 21-31-53
    Screenshot from 2022-02-01 21-45-28
  • Firefox has a checkbox:
    Screenshot from 2022-02-01 21-30-44
  • VSCode has a checkbox too:
    Screenshot from 2022-02-01 21-48-33

Update wording to use the one seen in web browsers and VS Code
@fcollonval
Copy link
MemberAuthor

fcollonval commentedFeb 2, 2022
edited
Loading

Thanks for the research@krassowski

Ind4860d1 I updated the wording toPause on exceptions forlabel and to a switching caption (so the button title will be switched).

Copy link
Member

@krassowskikrassowski left a 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'),
Copy link
Member

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:

Suggested change
label:trans.__('Pause onexceptions'),
label:trans.__('Pause onExceptions'),

but this is not a strong opinion.

@fcollonvalfcollonval deleted the fcollonval-patch-1 branchJuly 4, 2022 08:02
@github-actionsgithub-actionsbot locked asresolvedand limited conversation to collaboratorsJul 5, 2023
Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.

Reviewers

@krassowskikrassowskikrassowski approved these changes

Assignees

@fcollonvalfcollonval

Projects

None yet

Milestone

3.3.x

Development

Successfully merging this pull request may close these issues.

3 participants

@fcollonval@jasongrout@krassowski

[8]ページ先頭

©2009-2025 Movatter.jp