Quansight-Labs / jupyterlab-accessible-themes

♿️🎨 An access-centred implementation of the JupyterLab default themes
BSD 3-Clause "New" or "Revised" License
2 stars 1 forks source link

Add binder preview on PR #46

Closed steff456 closed 1 year ago

steff456 commented 1 year ago

This PR has the main objective of adding a preview in the PRs to check both installation and changes in a JupyterLab interface.

This PR,

As this PR is setting an action the idea is to squash all the commits once it is working.

steff456 commented 1 year ago

Screenshot of the last binder link:

image
trallard commented 1 year ago

I have not had time for a deep review - but just from a glance on the screenshot in the previous message I can see icons in black on top of the purple background... and I am going to go on a wild guess and mention

  1. I am sure that is not expected - in any case fallback icons should be the same lavender shade as in the sidebar
  2. black on that purple/indigo will not meet a 3:1 contrast so needs changing anyway
trallard commented 1 year ago

Question: is there a way to update the binder preview comment instead of creating a new comment on every commit?

This will/might get annoying pretty soon, i.e. too many notifications and noise - especially when folks like me who send looooads of small commits in quick succession work on PRs

steff456 commented 1 year ago

I have not had time for a deep review - but just from a glance on the screenshot in https://github.com/Quansight-Labs/jupyterlab-accessible-themes/pull/46#issuecomment-1504641448 I can see icons in black on top of the purple background... and I am going to go on a wild guess and mention

I'm checking this issue and it is only happening in binder, I'll take a look why this is happening.

Question: is there a way to update the binder preview comment instead of creating a new comment on every commit?

I'm going to see how to do it

github-actions[bot] commented 1 year ago

Binder :point_left: Launch a binder notebook on branch Quansight-Labs/jupyterlab-accessible-themes/add-preview Comment updated on 2023-04-17T20:21:25.981Z

steff456 commented 1 year ago

So apparently, the difference is that Binder is using JupyterLab verison 3.6.3 and my local environment was using 3.5.3. I'm not sure why, in the change of versions of 3.5.x to 3.6.x they decided to change the variable --jp-inverse-layout-colorX to --jp-inverse-layoutX.

For now, I duplicated the variables, in the way that the extension will work with both and it is now working in binder as expected 🎉

image
trallard commented 1 year ago

So apparently, the difference is that Binder is using JupyterLab verison 3.6.3 and my local environment was using 3.5.3. I'm not sure why, in the change of versions of 3.5.x to 3.6.x they decided to change the variable --jp-inverse-layout-colorX to --jp-inverse-layoutX.

Ok this raises another item to look at. Python 3.6 has reached its end of life and 3.7 is not far off see https://devguide.python.org/versions/.

I would suggest then targeting Python >= 3.8 and drop anything below that

steff456 commented 1 year ago

Hi @trallard,

The PostBuild script is a Python script atm - is there a particular reason this was preferred over a bash script? Pure curiosity as I usually use the latter and find it easier and it helps me avoid calling subprocesses

I'm using the template of binder for extensions and they are using a Python script instead of a bash one.. I don't have any preference but decided to take the template as it is.

The colours in the theme - did you use the ones in the trallard/pitaya_smoothie repo or the Jupyterlab one? either is fine, but if you used the latter one I might need to tweak some colours (nothing bad nor for you to take on)

I used the colors present in the JupyterLab extension repo, I'll create an issue to update the colors so they match the trallard/pitaya_smoothie repo.

steff456 commented 1 year ago

In this case., we might want to add a badge in the README file to point to the demo, otherwise, people will not even know this exists unless they look at the PRs

Apparently we already have a binder badge in the README on the main branch. It is currently broken, but it will work once we merge this PR.

image