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

Dark theme #5407

Merged
merged 52 commits into from
May 3, 2021
Merged

Dark theme #5407

merged 52 commits into from
May 3, 2021

Conversation

normanrz
Copy link
Member

@normanrz normanrz commented Apr 19, 2021

  • Implements a dark theme for webKnossos through a separate stylesheet
  • Tweaks the necessary styles for the UI components to adapt to both light and dark themes, i.e.
  • Reorganizes some stylesheets, e.g.
    • remove unused CSS styles
    • move spotlight stuff out of main.less
    • add a _utils.less stylesheet for exposing antd-variables as CSS variables
  • The charts in time_line_chart_view.js and statistic_view.js just got a white background, because the Google Chart module doesn't allow easy theming. Should be fixed in a follow-up PR.

TODO

  • persist mode (backend)
  • publication gallery
  • spotlight_view
  • welcome header
  • onboarding.js
  • dataset_upload_view.js
  • dataset_import_view.js
  • stacked_bar_chart.js
  • nml_upload_zone_container.js
  • viewport_status_indicator.js
  • dataset_position_view.js
  • dataset_settings_view.js
  • time_line_chart_view.js
  • experience_modal.js
  • dataset_action_view.js
  • toast.js
  • save_button.js
  • tracing_layout_view.js
  • dataset_info_tab.js
  • meshes_view.js
  • setting_input_view.js
  • share_modal.js
  • mapping_info_view.js
  • notifications
  • context menu
  • info tab
  • isosurface tab
  • tree tab
  • abstract tree tab (fixed with a white background)
  • segmentation tab
  • version restore
  • tracing-view navbar
  • Merge with Layout rework #5279
  • Merge with [WIP] Status bar #5369
  • Merge with Improve import+formats docs and add explanations to upload UI #5420

URL of deployed dev instance (used for testing):

Steps to test:


@normanrz normanrz self-assigned this Apr 25, 2021
@normanrz
Copy link
Member Author

Although there are still a few TODOs, I think the code is ready for a second pair of eyes.

@philippotto
Copy link
Member

I just pushed several tweaks which caught my eye during testing. From my point of view this PR can be merged. @fm3 Could you double-check the light theme to ensure that no embarrassing regression is introduced? The dark theme is marked as beta, so we are relatively safe there.

@fm3
Copy link
Member

fm3 commented Apr 29, 2021

Thanks for pinging me about the light theme :) I did notice some changes, only one of which I’d consider blocking:
light theme changes

  • hover color for welcome banner links is brighter (a little less legible but less impact than the recent font change)
  • error toast font color is red now (fine by me)
  • brush preview rendering has trippy new color scheme (Screenshot from Firefox. It’s better in chrome, but still definitely different from master. I think this one should be fixed)
    image
  • toggling the theme leads to “collapse”/“combination” of top menus (I just see a … instead of dashboard/administration/statistics…) as if there was too little space. It’s fixed on reload.
    image

@philippotto
Copy link
Member

Thanks for testing! I agree with your assessment of the blocking part.

  • brush preview rendering has trippy new color scheme (Screenshot from Firefox. It’s better in chrome, but still definitely different from master. I think this one should be fixed)
    image

Very interesting. I cannot reproduce this. It might be related to my comment here: #5407 (comment)
Could you check out this branch locally and set alpha to false just to check whether this fixes the rendering issue?

@fm3
Copy link
Member

fm3 commented Apr 30, 2021

Could you check out this branch locally and set alpha to false just to check whether this fixes the rendering issue?

Indeed, removing the alpha: true, line restores the master behavior

@normanrz
Copy link
Member Author

normanrz commented May 2, 2021

Thanks for your testing and suggestions!

  • hover color for welcome banner links is brighter (a little less legible but less impact than the recent font change)

I made the hover color a bit darker

  • brush preview rendering has trippy new color scheme (Screenshot from Firefox. It’s better in chrome, but still definitely different from master. I think this one should be fixed)
    image

I fixed this.

  • toggling the theme leads to “collapse”/“combination” of top menus (I just see a … instead of dashboard/administration/statistics…) as if there was too little space. It’s fixed on reload.
    image

I couldn't reproduce this :-(

@fm3
Copy link
Member

fm3 commented May 2, 2021

Excellent news :)

Concerning the last point, this seems to work fine in Chrome, but I can consistently reproduce the described problem in Firefox Windows+Linux. I don’t know how much of a problem it is, since reloading fixes the issue and most users won’t be switching themes very often.

@normanrz
Copy link
Member Author

normanrz commented May 2, 2021

Excellent news :)

Concerning the last point, this seems to work fine in Chrome, but I can consistently reproduce the described problem in Firefox Windows+Linux. I don’t know how much of a problem it is, since reloading fixes the issue and most users won’t be switching themes very often.

I could also reproduce it in Firefox and fixed it now :-)

@fm3
Copy link
Member

fm3 commented May 3, 2021

I could also reproduce it in Firefox and fixed it now :-)

Works for me 🎉

Copy link
Member

@philippotto philippotto left a comment

Choose a reason for hiding this comment

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

Excellent! Mergy-merge?

@normanrz normanrz merged commit b870799 into master May 3, 2021
@normanrz normanrz deleted the dark-theme branch May 3, 2021 08:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants