Removed debug switch, added bug button state update #10727

3coins · 2021-07-29T04:19:21Z

References

This PR fixes #10680.

Code changes

Add ARIA attributes, pressed and enabled states to toolbar button
Removed debug switch button from toolbar
Moved bug button before the kernel switcher
Updated debugger handler to show a new icon for pressed state
Updated debugger handler, so show debug button as disabled for kernels with no debugging support
Removed build script code that writes icon classes to deprecated.css
Updated kernel status toolbar icon styling
Added right padding to notebook toolbar to have some visual space for the kernel status icon.

User-facing changes

Backwards-incompatible changes

N/A

jupyterlab-dev-mode · 2021-07-29T04:19:28Z

Thanks for making a pull request to JupyterLab!

To try out this branch on binder, follow this link:

ellisonbg · 2021-07-29T17:14:39Z

I like the conceptual idea that the debugger button is related to the other kernel items. At the same time, I think this control might work better to the left of the kernel switcher UI. Historically, the kernel status indicator has been the right-most toolbar icon, and I think it makes sense to preserve that positioning. @3coins can you try it to the left of the kernel switcher?

ellisonbg

Approved from a UX perspective

3coins · 2021-07-30T17:17:57Z

I like the conceptual idea that the debugger button is related to the other kernel items. At the same time, I think this control might work better to the left of the kernel switcher UI. Historically, the kernel status indicator has been the right-most toolbar icon, and I think it makes sense to preserve that positioning. @3coins can you try it to the left of the kernel switcher?

Done, plz take a look.

krassowski

Thank you for working on this!

As a partially colorblind person I have to object to the current implementation as it is not accessible. We need a different icon for the active state, distinguished by more than color as proposed in Remove debug buttons in toolbar #10680 (comment). I don't care if it gets the run triangle icon "|>" added, is upside down, rotated or anything else - but it has to differ. Let's not introduce new accessibility issues :). Please let me know iff you need help with creating a new icon.
Would it be a good idea to make the tooltip ("'Enable / Disable Debugger'") state-specific, this is show either "Enable Debugger" or "Disable Debugger" depending on the state?
There is also a regression for screen readers (aria state which was conveyed by Switch toggle got removed without an alternative added).

packages/debugger/src/handler.ts

fcollonval · 2021-08-04T10:08:05Z

Thanks a lot for this one @3coins

I wonder if it could be better to always display the icon (with a status disabled if the kernel does not support debugger). The delay to display the debug icon when switching kernels feels awkward; see animation below. What do you think @ellisonbg ?

3coins · 2021-08-04T15:21:26Z

Thanks a lot for this one @3coins

I wonder if it could be better to always display the icon (with a status disabled if the kernel does not support debugger). The delay to display the debug icon when switching kernels feels awkward; see animation below. What do you think @ellisonbg ?

Thanks for that suggestion. I agree that it is clunky the way debug button is added into the page. @ellisonbg, let me know what you think having a disabled button there from UX perspective.

fcollonval

Thanks @3coins

I left some comments. Could you also address those two generic points:

methods should be documented (short sentence on what they are doing and what are their arguments and returned value)
please type the return value of functions
please type the private variables

buildutils/src/ensure-package.ts

fcollonval · 2021-08-19T06:23:05Z

packages/debugger/src/handler.ts

  const trans = translator.load('jupyterlab');
  const icon = new ToolbarButton({
    className: 'jp-DebuggerBugButton',
    icon: bugIcon,
-    tooltip: trans.__('Enable / Disable Debugger'),
+    tooltip: trans.__('Enable Debugger'),
+    ariaLabel: trans.__('Enable/Disable Debugger'),


@3coins I will drop the aria-label as the title has the same content and is changing depending on the button state (on the opposite of the aria-label)

fcollonval · 2021-08-19T06:25:17Z

packages/notebook/style/toolbar.css

+:root {
+  --jp-notebook-toolbar-padding: 2px 5px 2px 2px;
+}
+


fcollonval · 2021-08-19T06:27:39Z

packages/ui-components/src/components/toolbar.tsx

@@ -607,10 +625,57 @@ export class ToolbarButton extends ReactWidget {
  constructor(private props: ToolbarButtonComponent.IProps = {}) {
    super();
    addToolbarButtonClass(this);
+    this._enabled = props.enabled ?? true;
+    this._pressed = this._enabled === true ? props.pressed ?? false : false;


Small simplification

Suggested change

this._pressed = this._enabled === true ? props.pressed ?? false : false;

this._pressed = this._enabled && props.pressed;

fcollonval · 2021-08-19T06:31:33Z

packages/ui-components/src/components/toolbar.tsx

+  set pressed(value: boolean) {
+    if (value != this._pressed) {
+      this._pressed = value;
+      this.update();
+    }
+  }
+
+  get pressed() {
+    return this._enabled === true ? this._pressed : false;
+  }
+
+  set enabled(value: boolean) {
+    if (value != this._enabled) {
+      this._enabled = value;


You should enforce in setter that pressed cannot be set if the button is disabled. So the getter should point to the private value only (otherwise in further use, this._pressed could be different that this.pressed).

Suggested change

set pressed(value: boolean) {

if (value != this._pressed) {

this._pressed = value;

this.update();

}

}

get pressed() {

return this._enabled === true ? this._pressed : false;

}

set enabled(value: boolean) {

if (value != this._enabled) {

this._enabled = value;

set pressed(value: boolean) {

if (this.enabled && value !== this._pressed) {

this._pressed = value;

this.update();

}

}

get pressed() {

return this._pressed;

}

set enabled(value: boolean) {

if (value !== this._enabled) {

this._enabled = value;

if (!this._enabled) {

this._pressed = false;

}

fcollonval · 2021-08-19T06:34:09Z

packages/ui-components/test/toolbar.spec.ts

+          'ui-components:bug-dot'
+        );
+        widget.dispose();
+      });


Please add a test to check that disabled button cannot be pressed.

fcollonval · 2021-08-19T06:34:40Z

packages/ui-components/test/toolbar.spec.ts

+        expect(button.getAttribute('aria-disabled')).toEqual('true');
+        widget.dispose();
+      });
+    });


Please add a test that disabling a pressed button, unpressed it.

fcollonval · 2021-08-19T07:09:57Z

One additional comment I forgot in the review. The new button should be using the new toolbar customization feature.

See for example

* Removed debug switch * Moved debug button to left of kernel name * Added aria attributes, pressed and enabled states to ToolbarButton component * Disabled debug button when debugging is not supported

3coins · 2021-08-25T03:26:24Z

One additional comment I forgot in the review. The new button should be using the new toolbar customization feature.

See for example

@fcollonval
This requires a lot of changes as the button currently is embedded inside the handler, I will add this change to a separate PR. I have made all the other changes as per your suggestions.

For removing the icon css classes, here is the pull request - #10908

jtpio · 2021-08-27T11:50:50Z

Thanks for working on this, it looks great on Binder!

Is there any remaining items to fix, or could some of the comments above be addressed in separate PRs?

fcollonval

We can address my comment in a follow-up PR as suggested by @3coins => follow-up issue created #10939

jtpio

Thanks!

jtpio · 2021-09-01T09:01:11Z

Looks like a follow-up on the UI tests is needed after the merge: https://github.com/jupyterlab/jupyterlab/runs/3482183886

jtpio · 2021-09-01T09:27:57Z

Looks like a follow-up on the UI tests is needed after the merge: https://github.com/jupyterlab/jupyterlab/runs/3482183886

Opened #10982

meeseeksmachine · 2021-09-24T14:21:04Z

This pull request has been mentioned on Jupyter Community Forum. There might be relevant details there:

https://discourse.jupyter.org/t/how-to-toggle-a-toolbar-button-icon/10952/4

meeseeksmachine · 2021-09-27T19:38:25Z

This pull request has been mentioned on Jupyter Community Forum. There might be relevant details there:

https://discourse.jupyter.org/t/how-to-toggle-a-toolbar-button-icon/10952/5

Co-authored-by: Piyush Jain <pijain@amazon.com>

meeseeksmachine · 2021-10-17T20:49:42Z

This pull request has been mentioned on Jupyter Community Forum. There might be relevant details there:

https://discourse.jupyter.org/t/jupyterlab-ide-customization/11271/6

krassowski · 2021-10-17T20:52:16Z

We may have missed the editor toolbar, as per the report in https://discourse.jupyter.org/t/jupyterlab-ide-customization/11271/5. It indeed does not seem possible to debug standalone scripts anymore in 3.2. CC @3coins.

jtpio · 2021-10-18T08:13:04Z

Ah good catch, thanks @krassowski.

Let's open a new issue to track this?

github-actions bot added Design System CSS pkg:debugger tag:CSS For general CSS related issues and pecadilloes labels Jul 29, 2021

github-actions bot assigned 3coins Jul 29, 2021

blink1073 added the enhancement label Jul 29, 2021

blink1073 added this to the 4.0 milestone Jul 29, 2021

3coins force-pushed the remove-debug-switch branch 2 times, most recently from 29781b2 to 8c94974 Compare July 30, 2021 17:14

ellisonbg approved these changes Jul 30, 2021

View reviewed changes

krassowski requested changes Jul 31, 2021

View reviewed changes

packages/debugger/src/handler.ts Outdated Show resolved Hide resolved

github-actions bot added documentation pkg:apputils pkg:docmanager pkg:docregistry pkg:notebook pkg:statusbar pkg:toc pkg:ui-components labels Aug 4, 2021

3coins force-pushed the remove-debug-switch branch from 588434a to 2cd5090 Compare August 4, 2021 03:11

github-actions bot removed pkg:statusbar pkg:toc documentation pkg:notebook pkg:docregistry pkg:docmanager labels Aug 4, 2021

fcollonval requested changes Aug 19, 2021

View reviewed changes

Piyush Jain added 5 commits August 24, 2021 12:50

Added a new icon for debug button pressed state

02ad2d0

Removed build step to add new icon classes

dc587df

Removed debug switch, added aria, state updates

e067b6f

* Removed debug switch * Moved debug button to left of kernel name * Added aria attributes, pressed and enabled states to ToolbarButton component * Disabled debug button when debugging is not supported

Updated styles for kernel status, toolbar

b021776

Updated tooltip for toolbar debug button

bbc43fb

3coins force-pushed the remove-debug-switch branch from 2e4282b to bbc43fb Compare August 24, 2021 22:24

fcollonval self-requested a review August 27, 2021 13:43

fcollonval approved these changes Aug 27, 2021

View reviewed changes

fcollonval mentioned this pull request Aug 27, 2021

Use toolbar item factory for debug button #10939

Open

jtpio approved these changes Sep 1, 2021

View reviewed changes

jtpio merged commit 7c5092e into jupyterlab:master Sep 1, 2021

jtpio mentioned this pull request Sep 1, 2021

Galata: Update reference screenshots #10982

Merged

3coins pushed a commit to 3coins/jupyterlab that referenced this pull request Sep 29, 2021

Manual backport of PR jupyterlab#10727: Removed debug switch

a5b62ec

3coins mentioned this pull request Sep 29, 2021

Manual backport of PR #10727: Removed debug switch #11185

Merged

blink1073 pushed a commit that referenced this pull request Sep 29, 2021

Manual backport of PR #10727: Removed debug switch (#11185)

81b1a31

Co-authored-by: Piyush Jain <pijain@amazon.com>

krassowski mentioned this pull request Oct 18, 2021

It is no longer possible to debug scripts in 3.2 #11305

Closed

krassowski mentioned this pull request Jan 31, 2022

Pause on exception button does not show up #11965

Closed

2 tasks

github-actions bot added the status:resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion. label Apr 17, 2022

github-actions bot locked as resolved and limited conversation to collaborators Apr 17, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Removed debug switch, added bug button state update #10727

Removed debug switch, added bug button state update #10727

3coins commented Jul 29, 2021 •

edited by krassowski

jupyterlab-dev-mode bot commented Jul 29, 2021

ellisonbg commented Jul 29, 2021

ellisonbg left a comment

3coins commented Jul 30, 2021

krassowski left a comment •

edited

fcollonval commented Aug 4, 2021

3coins commented Aug 4, 2021

fcollonval left a comment

fcollonval Aug 19, 2021

fcollonval Aug 19, 2021

fcollonval Aug 19, 2021

fcollonval Aug 19, 2021

fcollonval Aug 19, 2021

fcollonval Aug 19, 2021

fcollonval commented Aug 19, 2021

3coins commented Aug 25, 2021

jtpio commented Aug 27, 2021

fcollonval left a comment •

edited

jtpio left a comment

jtpio commented Sep 1, 2021

jtpio commented Sep 1, 2021

meeseeksmachine commented Sep 24, 2021

meeseeksmachine commented Sep 27, 2021

meeseeksmachine commented Oct 17, 2021

krassowski commented Oct 17, 2021

jtpio commented Oct 18, 2021

	this._pressed = this._enabled === true ? props.pressed ?? false : false;
	this._pressed = this._enabled && props.pressed;

Removed debug switch, added bug button state update #10727

Removed debug switch, added bug button state update #10727

Conversation

3coins commented Jul 29, 2021 • edited by krassowski

References

Code changes

User-facing changes

Backwards-incompatible changes

jupyterlab-dev-mode bot commented Jul 29, 2021

ellisonbg commented Jul 29, 2021

ellisonbg left a comment

Choose a reason for hiding this comment

3coins commented Jul 30, 2021

krassowski left a comment • edited

Choose a reason for hiding this comment

fcollonval commented Aug 4, 2021

3coins commented Aug 4, 2021

fcollonval left a comment

Choose a reason for hiding this comment

fcollonval Aug 19, 2021

Choose a reason for hiding this comment

fcollonval Aug 19, 2021

Choose a reason for hiding this comment

fcollonval Aug 19, 2021

Choose a reason for hiding this comment

fcollonval Aug 19, 2021

Choose a reason for hiding this comment

fcollonval Aug 19, 2021

Choose a reason for hiding this comment

fcollonval Aug 19, 2021

Choose a reason for hiding this comment

fcollonval commented Aug 19, 2021

3coins commented Aug 25, 2021

jtpio commented Aug 27, 2021

fcollonval left a comment • edited

Choose a reason for hiding this comment

jtpio left a comment

Choose a reason for hiding this comment

jtpio commented Sep 1, 2021

jtpio commented Sep 1, 2021

meeseeksmachine commented Sep 24, 2021

meeseeksmachine commented Sep 27, 2021

meeseeksmachine commented Oct 17, 2021

krassowski commented Oct 17, 2021

jtpio commented Oct 18, 2021

3coins commented Jul 29, 2021 •

edited by krassowski

krassowski left a comment •

edited

fcollonval left a comment •

edited