Migrate to jupyter-ui-toolkit for Switch component #127
Conversation
* Blueprintjs that is used until now is overriding the default body font of lab. This commit fixes it by replace Switch component from blueprintjs to jupyter-ui-toolkit Signed-off-by: Mahendra Paipuri <[email protected]>
* To ensure dependencies are satisfied with JupyterLab Signed-off-by: Mahendra Paipuri <[email protected]>
* setup-python action does not support Python < 3.11 for MacOS runners based on Arm Signed-off-by: Mahendra Paipuri <[email protected]>
* Use venv for all steps in packaging workflow Signed-off-by: Mahendra Paipuri <[email protected]>
mahendrapaipuri
force-pushed
the
use_jupyter_ui_components
branch
from
31dedc6 to
02d3b12
Compare
|
Here is the binder link to test the PR -> https://mybinder.org/v2/gh/mahendrapaipuri/jupyterlab-topbar/use_jupyter_ui_components |
There was a problem hiding this comment.
Thanks @mahendrapaipuri for looking into this!
The new switch looks good. Maybe we should tweak the logic to take into account the current theme on page load?
| @@ -85,6 +85,11 @@ jobs: | |||
| py_cmd: python3 | |||
| - os: ubuntu | |||
| py_cmd: python | |||
| # actions/setup-python do not support MacOS runners based on ARM for Python < 3.11 | |||
| # https://github.com/actions/setup-python/issues/906 | |||
| exclude: | |||
There was a problem hiding this comment.
Wondering if we should remove the line architecture: 'x64' below in the actions/setup-python, and bump to actions/setup-python@v5?
Maybe we could then remove the extra source ./venv/bin/activate?
There was a problem hiding this comment.
Bumped actions/setup-python to v5. The virtual env stuff is due to MacOS ARM runners that need packages to be installed in virtual envs. To avoid conditional logic, I just made it default for all OS types.
| innerLabel="light" | ||
| innerLabelChecked="dark" | ||
| /> | ||
| <Switch onChange={onChange} checked={true}> |
There was a problem hiding this comment.
Should checked always be true?
Testing on Binder, the theme seems to be changed after a page refresh:
theme-toggler.webm
There was a problem hiding this comment.
Yes, you are correct. I over simplified the logic. Now I added the logic back to keep the state of the switch based on current theme.
|
Also the theme name used to be "inside" the switch component:
Not sure there is a way to do this with the component from
|
Signed-off-by: Mahendra Paipuri <[email protected]>
Signed-off-by: Mahendra Paipuri <[email protected]>
Yeah, there is no native way to embed labels inside the Switch component. Not sure if it is worth customizing the component. Maybe it makes more sense to address this issue in |
Signed-off-by: Mahendra Paipuri <[email protected]>
|
@jtpio Could you please take a look at it when you got some time? Cheers! |
Fixes #124
@jtpio I had some time to look at it. Let me know your thoughts!