-
Notifications
You must be signed in to change notification settings - Fork 16.4k
Add navbar personalization #56854
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 navbar personalization #56854
Conversation
|
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)
|
|
It generates a custom color palette where:
Some doubts I have:
For the future, when #55789 is merged we can add support for predefined themes and custom themes |
58d777a to
f52c95d
Compare
jason810496
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Thanks for the PR! LGMT for the API change.
The static check for spelling still need fix ( the CI failure will be handled in #56869, so no need to fix anything ).
We just need to add the missing words in docs/spelling_wordlist.txt. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, thanks for the PR!
I think the customization should be a whole theme object that 'chakra UI` understands. Either a css file or an object definition, so we can go beyond just 'navbar' or one palette customization. (customizing border radius, or anything else in the app, for specific components too.)
Sorry, do I need to do something from my end when you said "add the missing words in |
I thought about two possible values for
Agree for providing to the end-users full personalization of the UI but I think for the majority (I include myself here) changing only the color will be enough (DEV green, TST yellow, PRD red) |
jason810496
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, do I need to do something from my end when you said "add the missing words in docs/spelling_wordlist.txt"?
Yes, based on the error traceback in CI, we need to add the word grayscale in docs/spelling_wordlist.txt, thanks!
------------------------------ Error 2 ------------------------------
airflow-core/docs/howto/customize-ui.rst:78: (grayscale) will generate the same color palette (grayscale).
File path: /opt/airflow/airflow-core/docs/airflow-core/docs/howto/customize-ui.rst
Incorrect Spelling: 'grayscale'
Line with Error: ' will generate the same color palette (grayscale).'
====================================================================================================
f52c95d to
7368434
Compare
|
@odaneau-astro Would love your thoughts on this? For me, I was thinking about allowing a larger theme json config element. Also I am skeptical about us maintaining how color palettes are generated. I think that should be the domain of the person customizing the theme. We can still point them to useful tools. It also would be a slow development cycle to need to refresh the whole UI when you realize your base color wasn't just right. |
|
I like the simplicity of this solution. The implementation expands on my original idea in https://playcode.io/2464307 and fixes obvious flaws in my POC. I agree that we do want to expand customization options to other attributes, but we need to brainstorm a little more on what that would include. If we used a JSON env var with a single entry for navbar color for now, it would allow us to expand on it later. Personally, I would not offer users the ability to customize things component-by-component. It would add a maintenance burden for end users when we add new components in the future. I would also limit configuration attributes to essentials to avoid "config hell". Starting with accessibility requirements and corporate branding configs makes sense. Here are some that are top of mind:
Keep in mind that standardizing our existing codebase to use these will take time and effort. FYI, I also have this draft PR in the works to centralize component semantic tokens and make them rely more on brand colors: #56347 |
I don't have a strong opinion on maintaining the palette generation logic ourselves. We could very well use @Zinkue's generatePalette.ts logic in the playground (#55789) and output the hex color list for users to copy in their env var / airflow config. Then, only allow the long list of hex codes for 50-950 values in the JSON config. |
|
Many thanks for all your feedback! This how I understood the flow for changing the navbar color should be:
From my end I will break this PR in two work items:
If there will be no more comments, I will close this PR in favor of above's work items |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the follow up @Zinkue.
Sounds like a good plan. Indeed it will be easier to move forward with two different PRs for those two work items.
closes: #53443
Bring back Airflow 2 side/navbar personalization based on a user provided color (
themeconfiguration property)Screenshoots
Input

Output

Input

Output

Input

Output

^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named
{pr_number}.significant.rstor{issue_number}.significant.rst, in airflow-core/newsfragments.