search

jupyterlab / extension-examples

JupyterLab Extensions by Examples
BSD 3-Clause "New" or "Revised" License
454 stars 167 forks source link

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

Closed adpatter closed 3 years ago

adpatter commented 3 years ago

This PR is in reference to https://github.com/jupyterlab/extension-examples/issues/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 commented 3 years ago

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 commented 3 years ago

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

meeseeksmachine commented 3 years ago

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 commented 3 years ago

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.

adpatter commented 3 years ago

@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 commented 3 years ago

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

adpatter commented 3 years ago

@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.