Modify Signals to render a HTML button without React. #148

adpatter · 2021-02-12T18:38:22Z

This PR is in reference to #144.

Modify the Signals example to create an HTML button instead of a React button. I think this modification will make the documentation easier to follow for persons who don't know React.

Further, the Lumino Widgets were modified in order to produce the desired result shown in preview.png. The result of the current React implementation doesn't actually produce what is in the preview image.

CSS class naming conventions were followed.

meeseeksmachine · 2021-02-12T18:40:10Z

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

https://discourse.jupyter.org/t/is-usage-of-react-necessary-for-writing-jupyterlab-extensions-effectively/7825/6

adpatter · 2021-02-13T03:41:48Z

Can anyone help me understand why the PR is failing 2 Checks?

meeseeksmachine · 2021-02-13T03:44:35Z

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

https://discourse.jupyter.org/t/can-anyone-help-me-understand-why-my-pr-is-failing-2-checks/7932/1

fcollonval · 2021-02-13T14:56:41Z

Thanks @adpatter

The errors come from the snippets in the README. To ensure the code and the README are aligned we are using a tool call embedme that copy the code snippets within the README. And in the CI, we are checking the code and snippets are aligned.

To update the README, you need to execute jlpm run embedme in the examples root folder. Then you can push the updated README.

More details - in the CI logs you can see:

  Analysing basics/signals/README.md...
   basics/signals/README.md#L26-L30 Embedded 1 lines from file src/button.ts#L36-L36
   basics/signals/README.md#L34-L40 Embedded 3 lines from file src/button.ts#L15-L17
   basics/signals/README.md#L45-L49 Embedded 1 lines from file src/panel.ts#L29-L29
   basics/signals/README.md#L53-L57 Embedded 1 lines from file src/button.ts#L28-L28
   basics/signals/README.md#L72-L92 Embedded 17 lines from file src/button.ts#L8-L24
   basics/signals/README.md#L98-L104 Embedded 3 lines from file src/button.ts#L26-L28
   basics/signals/README.md#L109-L113 Embedded 1 lines from file src/button.ts#L30-L30
   basics/signals/README.md#L124-L131 No changes required, already up to date
   basics/signals/README.md#L139-L155 Embedded 13 lines from file src/panel.ts#L18-L30
   basics/signals/README.md#L159-L163 Embedded 1 lines from file src/panel.ts#L29-L29
   basics/signals/README.md#L178-L182 Embedded 1 lines from file src/panel.ts#L32-L32
   basics/signals/README.md#L187-L199 Embedded 9 lines from file src/panel.ts#L32-L40

So all snippets in the README needs to be updated except the one in lines 124-131.

fcollonval

@adpatter I added some comments and suggestions.

fcollonval · 2021-02-13T15:07:17Z

basics/signals/README.md

+      this._stateChanged.emit(this._count);
+    });
+  }
+  //...
 }
 ```

 `ButtonWidget` also contain a private attribute `_count` of type `ICount`.


Suggested change

`ButtonWidget` also contain a private attribute `_count` of type `ICount`.

`ButtonWidget` also contains a private attribute `_count` of type `ICount`.

Taking the chance to correct that one 😉

Fixed as requested.

fcollonval · 2021-02-13T15:09:59Z

basics/signals/package.json

-    "@lumino/disposable": "^1.4.3"
+    "@lumino/disposable": "^1.4.3",
+    "@lumino/signaling": "^1.3.3",
+    "@lumino/widgets": "^1.3.3"


Suggested change

"@lumino/widgets": "^1.3.3"

"@lumino/widgets": "^1.16.1"

Alignment with jupyterlab dependency constrain

Fixed as requested. Thanks for the guidance.

fcollonval · 2021-02-13T15:11:14Z

basics/signals/package.json

    "@jupyterlab/application": "^3.0.0-rc.15",
+    "@jupyterlab/apputils": "^3.0.3",
    "@jupyterlab/launcher": "^3.0.0-rc.15",
    "@jupyterlab/mainmenu": "^3.0.0-rc.15",
    "@jupyterlab/translation": "^3.0.0-rc.15",


Suggested change

"@jupyterlab/application": "^3.0.0-rc.15",

"@jupyterlab/apputils": "^3.0.3",

"@jupyterlab/launcher": "^3.0.0-rc.15",

"@jupyterlab/mainmenu": "^3.0.0-rc.15",

"@jupyterlab/translation": "^3.0.0-rc.15",

"@jupyterlab/application": "^3.0.3",

"@jupyterlab/apputils": "^3.0.3",

"@jupyterlab/launcher": "^3.0.3",

"@jupyterlab/mainmenu": "^3.0.3",

"@jupyterlab/translation": "^3.0.3",

it is a good practice to be consistent on the jupyterlab dependencies.

Fixed as requested.

fcollonval · 2021-02-13T15:15:47Z

basics/signals/src/panel.ts

-/**
- * The class name added to console panels.
- */
-const PANEL_CLASS = 'jp-tutorial-view';


Could you keep the class name as constant as this is to align with JupyterLab practice?

Fixed as requested. However, I noticed that the documentation seemed to indicated that they were moving away from that practice: "CSS classnames are defined inline in the code. We used to put them as all caps file-level consts, but we are moving away from that." (https://jupyterlab.readthedocs.io/en/stable/developer/css.html)

Thanks for pointing this out - I miss that information. I have opened a new issue to conform to that principle. So we can merge this one.

fcollonval · 2021-02-13T15:16:20Z

basics/signals/src/button.ts

+     * The class name, jp-ButtonWidget, follows the CSS class naming
+     * convention for classes that extend lumino.Widget.
+     */
+    this.addClass('jp-ButtonWidget');


Could you extract the class name as constant to align with JupyterLab practice?

Fixed as requested.

fcollonval · 2021-02-13T15:16:55Z

basics/signals/src/button.ts

+  constructor(options = { node: document.createElement('button') }) {
+    super(options);
+
+    this.node.textContent = 'Clickme';


What about

Suggested change

this.node.textContent = 'Clickme';

this.node.textContent = 'Click me';

Fixed as requested.

adpatter · 2021-02-13T17:52:56Z

@fcollonval Thank you for reviewing my code and providing guidance. In particular, thank you for the guidance on how to choose dependency versions - it makes sense to me now. I made all of the requested corrections to the code.

It looks like the PR has passed all the checks. Thank you.

adpatter · 2021-02-13T19:12:59Z

A .vscode file snuck in there, so I pushed again.

adpatter · 2021-02-13T20:37:10Z

@fcollonval It took me awhile to figure out how embedme works. However, I think I have all the mappings correct now - I apologize if this caused you any inconvenience. I will be more careful about the mappings in the future. Thank you.

fcollonval

LGTM ! Thanks @adpatter

fcollonval requested changes Feb 13, 2021

View reviewed changes

adpatter marked this pull request as draft February 13, 2021 19:43

Modify signals to render an HTML button.

fc9611f

adpatter marked this pull request as ready for review February 13, 2021 20:33

fcollonval mentioned this pull request Feb 14, 2021

Align CSS classnames definition #149

Open

Minor update to align README with code

8098d3c

fcollonval approved these changes Feb 14, 2021

View reviewed changes

fcollonval merged commit ca71190 into jupyterlab:master Feb 14, 2021

jtpio mentioned this pull request Feb 15, 2021

Flatten the examples folder structure #146

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modify Signals to render a HTML button without React. #148

Modify Signals to render a HTML button without React. #148

adpatter commented Feb 12, 2021 •

edited

Loading

meeseeksmachine commented Feb 12, 2021

adpatter commented Feb 13, 2021

meeseeksmachine commented Feb 13, 2021

fcollonval commented Feb 13, 2021

fcollonval left a comment

fcollonval Feb 13, 2021

adpatter Feb 13, 2021

fcollonval Feb 13, 2021

adpatter Feb 13, 2021

fcollonval Feb 13, 2021

adpatter Feb 13, 2021

fcollonval Feb 13, 2021

adpatter Feb 13, 2021

fcollonval Feb 14, 2021

fcollonval Feb 13, 2021

adpatter Feb 13, 2021

fcollonval Feb 13, 2021

adpatter Feb 13, 2021

adpatter commented Feb 13, 2021

adpatter commented Feb 13, 2021 •

edited

Loading

adpatter commented Feb 13, 2021 •

edited

Loading

fcollonval left a comment

	`ButtonWidget` also contain a private attribute `_count` of type `ICount`.
	`ButtonWidget` also contains a private attribute `_count` of type `ICount`.

	this.node.textContent = 'Clickme';
	this.node.textContent = 'Click me';

Modify Signals to render a HTML button without React. #148

Modify Signals to render a HTML button without React. #148

Conversation

adpatter commented Feb 12, 2021 • edited Loading

meeseeksmachine commented Feb 12, 2021

adpatter commented Feb 13, 2021

meeseeksmachine commented Feb 13, 2021

fcollonval commented Feb 13, 2021

fcollonval left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adpatter commented Feb 13, 2021

adpatter commented Feb 13, 2021 • edited Loading

adpatter commented Feb 13, 2021 • edited Loading

fcollonval left a comment

Choose a reason for hiding this comment

adpatter commented Feb 12, 2021 •

edited

Loading

adpatter commented Feb 13, 2021 •

edited

Loading

adpatter commented Feb 13, 2021 •

edited

Loading