Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for JupyterLab Dark theme #706

Merged
merged 12 commits into from
Jul 7, 2020

Conversation

vabarbosa
Copy link
Contributor

@vabarbosa vabarbosa commented Jul 1, 2020

Updated Elyra components to better support and align with JupyterLab Dark & Light themes

  • Code Snippet sidebar widget
  • Code Snippet icon (in sidebar)
  • Python Runner console output (scroll buttons)
  • Pipeline Editor icon (in launcher & filebrowser)
  • Elyra logo
  • Dialogs (ExpandableErrorDialog)
  • Pipeline Editor (i.e., @elyra/canvas).

Fixes #625

image

image

image

image

image

Developer's Certificate of Origin 1.1

   By making a contribution to this project, I certify that:

   (a) The contribution was created in whole or in part by me and I
       have the right to submit it under the Apache License 2.0; or

   (b) The contribution is based upon previous work that, to the best
       of my knowledge, is covered under an appropriate open source
       license and I have the right under that license to submit that
       work with modifications, whether created in whole or in part
       by me, under the same open source license (unless I am
       permitted to submit under a different license), as indicated
       in the file; or

   (c) The contribution was provided directly to me by some other
       person who certified (a), (b) or (c) and I have not modified
       it.

   (d) I understand and agree that this project and the contribution
       are public and that a record of the contribution (including all
       personal information I submit with it, including my sign-off) is
       maintained indefinitely and may be redistributed consistent with
       this project or the open source license(s) involved.

Add support for dark theme in code snippets sidebar
Fix dark mode theme support in Python Console Output
Update the Elyra logo and Pipeline Editor icons to better support dark theme
support dark theme in the DockPanel background
@ajbozarth
Copy link
Member

Took a quick look and this is looking great so far, I'll do a full review once I'm back on Monday.

@lresende
Copy link
Member

lresende commented Jul 2, 2020

Does this also address #683 ?

@vabarbosa
Copy link
Contributor Author

Does this also address #683 ?

this updates how the code snippet icon is displayed (themed). it however, does not add the icon to the code snippet header (#683). i expected that to be its own PR but if you are OK with it i can include it in this PR as well. with this PR the icon properly displays in Light and Dark modes so once it does get included in the header it show properly show when switching themes.

Update the expandable error dialog to properly display in dark theme
@lresende
Copy link
Member

lresende commented Jul 2, 2020

Does this also address #683 ?

this updates how the code snippet icon is displayed (themed). it however, does not add the icon to the code snippet header (#683). i expected that to be its own PR but if you are OK with it i can include it in this PR as well. with this PR the icon properly displays in Light and Dark modes so once it does get included in the header it show properly show when switching themes.

It's ok to be it's own pr, just that I saw some icon related changes and thought it was covering that.

@marthacryan
Copy link
Member

Just tried this locally and it worked great for me! Should we open an issue for pipeline editor / reach out to canvas about adding a dark mode setting?

Copy link
Member

@marthacryan marthacryan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Update pipeline editor to support changing dark/light themes
@ajbozarth
Copy link
Member

I'll be taking a look at this later today

@vabarbosa vabarbosa marked this pull request as ready for review July 6, 2020 18:06
@vabarbosa
Copy link
Contributor Author

vabarbosa commented Jul 6, 2020

@ajbozarth all components have been updated. see screenshots in the description. the one remaining issue is when adding a Pipeline Editor icon to the canvas toolbar it is displayed using <img> instead of <svg> so colors cannot be updated dynamically via CSS.

as of elyra-ai/canvas#106 (Jun 1, 2020) JSX can be provided as an icon to the canvas toolbar so upgrading to recent version of canvas should help resolve this Pipeline Editor toolbar icon issue. however, short of upgrading canvas we may need to either

  • provide multiple alternate images for light/dark themes

or

  • keep all the Pipeline Editor toolbar icons blue (in both Light/Dark themes), but i don't believe the blue icon on black toolbar background provides an accessible/acceptable contrast ratio

(will investigate further)

@vabarbosa
Copy link
Contributor Author

vabarbosa commented Jul 6, 2020

@marthacryan we may need to coordinate this with your metadata #589 PR to make sure the new pages/dialogs also support changing between Light and Dark themes

@vabarbosa
Copy link
Contributor Author

Just tried this locally and it worked great for me! Should we open an issue for pipeline editor / reach out to canvas about adding a dark mode setting?

@marthacryan thanks. i was able to work around the canvas support for dark mode and Pipeline Editor should now support switching between light and dark JupyterLab themes. however, the update could have been easier/quicker if canvas did have a support for switching (CSS) themes dynamically. it may be worth opening an issue with them for future consideration.

Copy link
Member

@karlaspuldaro karlaspuldaro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! LGTM
Just a comment, maybe "Run pipeline" icon should stay blue on dark mode to be consistent with the other buttons added on canvas toolbar from elyra?

@vabarbosa
Copy link
Contributor Author

vabarbosa commented Jul 6, 2020

Awesome! LGTM
Just a comment, maybe "Run pipeline" icon should stay blue on dark mode to be consistent with the other buttons added on canvas toolbar from elyra?

@karlaspuldaro thanks. yeah, that is a discussion (#706 (comment)) i wanted to have and get people's opinion. until we can update canvas (and use React components as icons) the simplest solution is to go with blue icons. the downside to that is the blue icon on black toolbar background may be difficult for some to see.

Copy link
Member

@ajbozarth ajbozarth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple comments, but this looks great over all.

As for the discussion around the canvas toolbar icon colors. I believe if we intend to update to latest canvas "soon" then leaving it blue for now (and opening an issue for the "know bug") is fine (at least its better than no dark mode). And iiuc canvas does not "actually" support dark mode, you just copied and overrode their css. If that is the case I believe you should open a PR on the canvas repo to include you dark mode support. Then, hopefully, we can get it into a release alongside the new jsx icon support that elyra can update to.

@vabarbosa
Copy link
Contributor Author

vabarbosa commented Jul 6, 2020

A couple comments, but this looks great over all.

As for the discussion around the canvas toolbar icon colors. I believe if we intend to update to latest canvas "soon" then leaving it blue for now (and opening an issue for the "know bug") is fine (at least its better than no dark mode).

alright, will keep the toolbar icons as blue

And iiuc canvas does not "actually" support dark mode, you just copied and overrode their css. If that is the case I believe you should open a PR on the canvas repo to include you dark mode support. Then, hopefully, we can get it into a release alongside the new jsx icon support that elyra can update to.

yes, canvas does not support a dark mode (or switching CSS themes dynamically). they do recommend importing the canvas scss files and overriding the variables to create your own theme: https://github.com/elyra-ai/canvas/wiki/5.0-Styling
however Elyra does not use Sass which means a Sass loader would need to be added to the build to process/compile the Sass to CSS and then import into JupyterLab. from JupyterLab docs:

Note that if you want to use SCSS, SASS, or LESS files, you must compile them to CSS and point JupyterLab to the CSS files.

i figured editing/overriding the CSS would be quicker then trying to add Sass and overcomplicating the issue.

vabarbosa added 2 commits July 6, 2020 19:01
per review comment keep the toolbar icons blue in color
Remove attributes which are not color related
@vabarbosa
Copy link
Contributor Author

vabarbosa commented Jul 6, 2020

I assume this change is because you're editing the file to support dark mode? Is there a way to override the css value in our own file rather than copying and editing a external library's one? If not could you annotate the diff in this PR detailing what you changed in the file?

this is not an external file from canvas. i created the new canvas.css file just to keep all the changes needed for canvas in a single document. i could add them into the existing index.css instead but i believe it is better left separate. and if canvas does change it default theme or adds support for dynamically switching themes, most of the work needed for Elyra would be contained in this file.

the new canvas.css file added here does not change any of the values for the default canvas theme. rather it places all color values into CSS variables so they can easily be switched when changing themes. the file only contains color updates (i.e., fill, stroke, color, background, border). the colors of the following canvas components are updated in this file when switching themes.

  • comment box
  • node
  • toolbar
  • properties modal
  • tooltip
  • context menu

@vabarbosa
Copy link
Contributor Author

vabarbosa commented Jul 6, 2020

ooooo, pretty, I like it. and you should definitely open a PR fixing this in Lab

opened an issue in JupyterLab to fix it: jupyterlab/jupyterlab#8652

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update Elyra to support Dark Mode
5 participants