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 prototype version dark mode for Airflow UI #39355

Merged
merged 22 commits into from
Jun 26, 2024
Merged

Conversation

YounHS
Copy link
Contributor

@YounHS YounHS commented May 2, 2024

related: #11334

We've been seeing requests for a dark mode feature in the Airflow UI for a while now.

I tested it using docker-compose.yaml.

After testing, I was able to set dark mode and light mode.

The only thing I noticed is that after switching to dark mode, the light theme is visible for a very short time when the page is updated.

I think there should be a way to smooth this out in the future.

As-Is

  • This picture is just extract from airflow github readme.
  • I want to emphasize with this illustration that the dark/light mode toggle was not originally there.

image

To-Be

  • Dark Mode
    다크모드_캡처

  • Light Mode
    라이트모드_캡처

You can toggle the dark/light mode by clicking the crescent icon on the right side of the navigation bar.

If you changed the color of the navigation bar via a setting like AIRFLOW__WEBSERVER__NAVBAR_COLOR, the dark/light mode will be applied accordingly.


FIX

  • If Dark Mode is running, a moon will appear, and if Light Mode is running, a sun will appear.

@boring-cyborg boring-cyborg bot added area:UI Related to UI/UX. For Frontend Developers. area:webserver Webserver related Issues labels May 2, 2024
Copy link

boring-cyborg bot commented May 2, 2024

Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contributors' Guide (https://github.com/apache/airflow/blob/main/contributing-docs/README.rst)
Here are some useful points:

  • Pay attention to the quality of your code (ruff, mypy and type annotations). Our pre-commits will help you with that.
  • In case of a new feature add useful documentation (in docstrings or in docs/ directory). Adding a new operator? Check this short guide Consider adding an example DAG that shows how users should use it.
  • Consider using Breeze environment for testing locally, it's a heavy docker but it ships with a working Airflow and a lot of integrations.
  • Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
  • Please follow ASF Code of Conduct for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
  • Be sure to read the Airflow Coding style.
  • Always keep your Pull Requests rebased, otherwise your build might fail due to changes not related to your commits.
    Apache Airflow is a community-driven project and together we are making it better 🚀.
    In case of doubts contact the developers at:
    Mailing List: [email protected]
    Slack: https://s.apache.org/airflow-slack

Copy link
Contributor

@dirrao dirrao left a comment

Choose a reason for hiding this comment

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

Nice to see dark mode feature. The screen shot that you shared doesn't show the text properly. is it during switch mode. are there any other screenshots?

@YounHS
Copy link
Contributor Author

YounHS commented May 2, 2024

Nice to see dark mode feature. The screen shot that you shared doesn't show the text properly. is it during switch mode. are there any other screenshots?

Hi @dirrao The captured screen contains sensitive information, so we've mosaicked it, thank you for your understanding! :)

And
As you said, you can toggle between dark and light mode by toggling the moon icon right on navigation bar. I've added a capture screen for light mode!

@uranusjr
Copy link
Member

uranusjr commented May 3, 2024

So in the dark mode screenshot, text is actually white (like in the nav bar) and it being very extremely low contrast is just a side effect of the mosaic? It would be helpful if there’s a non-filtered example. I have some worry about the blue used in links, for example.

@ryanahamilton
Copy link
Contributor

The screenshots illustrate that this will certainly be an appealing feature—I really wish the execution were as easy as this… The path that we should take (IMO) to enabling color mode in a sustainable manner is to migrate the rest of the UI to React. Within the React app, Chakra, the UI framework being employed, already has a robust color mode support baked in. The problem is that the React app is nested within the aged Flask+Bootstrap app and the two frameworks don't share a common color mode system. I'm sure it would be possible to bridge the divide/logic between them, but one would have to determine if that is worth the effort versus investing time in migrating more/the remaining UI to the React app. Once the UI is entirely in React, enabling it will trivial.

@YounHS YounHS requested a review from dirrao May 3, 2024 12:04
@YounHS
Copy link
Contributor Author

YounHS commented May 3, 2024

hi @uranusjr. I removed the mosaicked part and created a test DAG to recapture it :)

@jscheffl
Copy link
Contributor

jscheffl commented May 4, 2024

I am 80% okay with this interim. Anyway when moving to full-React we need to rework this thing. Found no problem on all existing pages with Firefox+Chromium on Ubuntu.

What irritates me is really that flickering during page load. I also checked with PR #39345 merged-in but this also did not help. Can this be improved?

I checked when setting my browser (Firefox as well as Chromium) to Dark mode explicitly, I'd expect the browser setting initially is picked-up. It is not. Can you add this?

@YounHS
Copy link
Contributor Author

YounHS commented May 9, 2024

Hello, @jscheffl I fix the flickering problem. for this solution, I add custom function in views.py. It just a prototype, so please excuse the messy code.

Add.
I test flickering solution by chrome browser! maybe firefox also run well i think.

airflow/www/views.py Outdated Show resolved Hide resolved
airflow/www/templates/appbuilder/navbar.html Outdated Show resolved Hide resolved
@YounHS YounHS requested a review from jscheffl May 11, 2024 07:58
Copy link
Contributor

@jscheffl jscheffl left a comment

Choose a reason for hiding this comment

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

Thanks for the re-work. Flickering is gone now. Almost looks good for me but I was clicking through the UI with the changes and found small new problems:

  • Login page is not displaying in dark mode. Is always light
  • Pages below menus "Security", "Browse" and "Admin" are all always in Light mode

Otherwise all "major" content looks good.

I see you added auto-detection of browser preference, but also via clearing cookies was not able to have this working. Was always showing light mode.

@YounHS
Copy link
Contributor Author

YounHS commented May 12, 2024

@jscheffl
sry, I checked this prob!

I initially thought that I would only need to modify the airflow/www side, but for "security", "browse", and "admin", modifying views.py would not fix the problem.

This would require modifying the flask_appbuilder side, which I couldn't do.

So, I solved this by deleting the custom classes I added to views.py and simply adding the script tag to the head of main.html.

@YounHS YounHS requested a review from jscheffl May 12, 2024 04:43
Copy link
Contributor

@vincbeck vincbeck left a comment

Choose a reason for hiding this comment

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

The code LGTM and I tested it in my env on multiple pages and it looks good! Just the error pages (404 or when there is an exception) are excluded from this change but I guess it is alright.

@YounHS YounHS requested a review from bbovenzi June 20, 2024 01:06
@YounHS YounHS requested a review from bbovenzi June 24, 2024 00:45
@jscheffl
Copy link
Contributor

Oh, no just another small static check glitch. Else finally looks OK.

Copy link
Contributor

@jscheffl jscheffl left a comment

Choose a reason for hiding this comment

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

Uff, oh. This was a long path. Tested manually again as regression, finally code looks good and CI green in a moment. LGTM!

Thanks for the consistent engagement making it final!

@jscheffl jscheffl merged commit f97c297 into apache:main Jun 26, 2024
49 checks passed
Copy link

boring-cyborg bot commented Jun 26, 2024

Awesome work, congrats on your first merged pull request! You are invited to check our Issue Tracker for additional contributions.

@potiuk
Copy link
Member

potiuk commented Jun 26, 2024

WOOOOOOOOHOOOOO!!!!

@vincbeck
Copy link
Contributor

+1! Very good and impactful contribution!

@potiuk
Copy link
Member

potiuk commented Jun 26, 2024

What's the Hashtag we use to mark PR a candidate for PR of the month @jedcunningham ?

@jedcunningham
Copy link
Member

@potiuk: #protm (source, hash is optional, but more fun 😄 )

@potiuk
Copy link
Member

potiuk commented Jun 26, 2024

So ... It's DONE... Since you mentioned it :)

@potiuk
Copy link
Member

potiuk commented Jun 26, 2024

One more small comment. I just run it and I think the background is a little too dark. Most of the dark-mode apps I have result with dark-but-not-black background and when I tried Airflow in dark mode it looks pitch-black.

@jscheffl
Copy link
Contributor

One more small comment. I just run it and I think the background is a little too dark. Most of the dark-mode apps I have result with dark-but-not-black background and when I tried Airflow in dark mode it looks pitch-black.

Now whining post-merge... there had been sufficient time to test :-D but also until 2.10 still time to update CSS if you think it is tooo dark.

@potiuk
Copy link
Member

potiuk commented Jun 26, 2024

image

@potiuk
Copy link
Member

potiuk commented Jun 26, 2024

(and to excuse myself - I DID try it at some point in time, but it did not work then :D )

@bbovenzi
Copy link
Contributor

One more small comment. I just run it and I think the background is a little too dark. Most of the dark-mode apps I have result with dark-but-not-black background and when I tried Airflow in dark mode it looks pitch-black.

Yeah, that's why I wanted the experimental flag. All this does it invert the colors vs creating a true dark theme. It will be better in Airflow 3.0 :)

@utkarsharma2 utkarsharma2 added the type:new-feature Changelog: New Features label Jul 1, 2024
@utkarsharma2 utkarsharma2 added this to the Airflow 2.10.0 milestone Jul 1, 2024
romsharon98 pushed a commit to romsharon98/airflow that referenced this pull request Jul 26, 2024
* ADD prototype version dark mode for Airflow UI

* fix flickering when refresh page

* clean up commented code

* indent fix tab to 2 indent

* add comment for linter

* fix to simple code, but to require refactoring in the future

* flickering fix optimization

* lint fix

* fix lint

* final lint error fix

* dark/light mode icon toggle

* add experimental element & auto detect to localstorage + user actions

* fix eventListener logic

* fix lint error

---------

Co-authored-by: hsyoun <[email protected]>
Co-authored-by: Jens Scheffler <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:UI Related to UI/UX. For Frontend Developers. area:webserver Webserver related Issues type:new-feature Changelog: New Features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants