Closed giaroc closed 7 months ago
mind fixing the conflicts? seems reasonable to me and I personally would use the high contrast mode
Hi @codefromthecrypt, I have performed a rebase, but subsequently had to integrate some changes.
locale.ts
since it is no longer used for storing the selected language in local storage, I preferred to rename it to theme.ts
.Please let me know if there are any further changes to be made and what you think in general. Thank you!
looks like you need to run npm run lint
per build fail. one way to ensure it will pass is call ./mvnw clean install -pl :zipkin-lens
from the workspace root
looks like you need to run
npm run lint
per build fail. one way to ensure it will pass is call./mvnw clean install -pl :zipkin-lens
from the workspace root
Now should be fixed:
Thanks I tried this locally and looks good. The main question I have which way to move forward:
I'm not familiar with projects having high contrast modes and what they name them as. If we go with option 2. we should cite a few famous projects, how they handle this, and how they name it. For example, maybe elasicsearch, grafana, or others probably have this request as their community size is quite big.
Thanks I tried this locally and looks good. The main question I have which way to move forward:
1. Change light mode to high contrast (to have one mode) * I personally prefer this as it is simpler to maintain 2. have separate light mode and high contrast mode * decide if "high contrast mode" is natural english, as well update the other languages for that button.
I'm not familiar with projects having high contrast modes and what they name them as. If we go with option 2. we should cite a few famous projects, how they handle this, and how they name it. For example, maybe elasicsearch, grafana, or others probably have this request as their community size is quite big.
Other products generally only have Light and Dark modes. However, it is worth considering that they are probably more focused on accessibility themes, being widely used products. Windows 10/11 has introduced the "Contrast" mode at the operating system level, but it follows quite specific and very different guidelines from those I followed for the "High Contrast" theme. In fact, high contrast themes generally do not have a wide color palette and prefer black and white (or yellow) as colors.
Here is a great article that explains the benefits of the high contrast theme: https://uxdesign.cc/high-contrast-when-you-think-the-dark-mode-is-enough-d190218d4bba
With that said, the high contrast theme I proposed slightly improves the background text contrast for colored elements, compared to the standard theme.
I don't see major difficulties in maintaining the contrast theme as it is now, being very similar to the light theme (maintaining the dark theme is much more difficult).
I preferred to keep a separate theme because I did not want to drastically change the original colors of the product. For example, the contrast theme has a more limited number of colors for high contrast services with the text and also contrast between them, but honestly for a person who does not have disabilities, the original color palette has nicer colors to look at.
A third option could be a hybrid one, which only retains the light and dark modes and slightly modifies the light one:
It should be noted that if a service has a sufficiently long name, in the Traces section, the service name is not fully visible within the colored label. In this case, the choice of color can be marginal.
So, to clarify the problem of maintenance in the UI is historical. For individuals it is not necessarily an issue to add a function at the time they add it. However, most people who work on the UI stop working after a year or two. This leaves works for unknown others, and the larger the codebase grows this becomes an issue. This is particularly troublesome in UI as the majority of devs haven't even adjusted a color at all.
Let's go with your hybrid option 3. When we release the version we can say if you don't like the colors let us know. Then, we can revert and justify more logic. Thanks for the help!
So, to clarify the problem of maintenance in the UI is historical. For individuals it is not necessarily an issue to add a function at the time they add it. However, most people who work on the UI stop working after a year or two. This leaves works for unknown others, and the larger the codebase grows this becomes an issue. This is particularly troublesome in UI as the majority of devs haven't even adjusted a color at all.
Let's go with your hybrid option 3. When we release the version we can say if you don't like the colors let us know. Then, we can revert and justify more logic. Thanks for the help!
Ok, I have made the changes, keeping Light and Dark. Do you think a translation for "Light" and "Dark" is necessary, or should we leave it in English for all languages? I'm not sure if perhaps Chinese needs to be translated, but I noticed in the translation file that some "international" terms have remained (spans, trace, etc.).
I for some reason thought we had an existing dark mode, maybe it was lost when we redid the UI, or perhaps it never existed. For this reason, I was a bit surprised to still see a lot of code here even when simplifying to two themes. However, now I can see that this change is needed and well described in the README.
That said, we still have a decision to make as the top bar is cluttered, and unlike most dashboards:
Most of the time, there is a settings button or implicitly if you click the logo to drag settings on the left. We cannot continue to add stuff into the top bar, and this feature dominates the top bar more than zipkin's core functions.
I think there are at least 3 ways out of this, in complexity order
The reason I'm not asking for comments and testing now, is to reduce your burden during revisions. Once this is in final shape, we'll need to add unit tests for selection logic, or anything that is likely to break if the UI component library switched later. (Historically, those who create features in the UI are not around to maintain them. This has resulted in me spending literally hours on small things I don't understand)
Yes, I actually agree about the topbar. Personally, I always prefer having the option to choose, and generally, most products, whether they are well-known products like Zipkin or those that have a completely different use case, allow you to change the option at will (or might add an option that follows your browser settings). Which path would you prefer to take? As for option 2 or 3, the second would require very little work. The third, however, would necessitate some reworking of the topbar, so there would be a few more lines of code involved. Option 2 could potentially be integrated while also reducing the space dedicated to the language switch selector. Often, only the ISO 639-1 Code or ISO 639-2 Code is used to display the selected language (EN, ES, IT, DE, FR, etc.).
This choice is more in your court as I am happy with options 2 or 3. It boils down to work involved and that there should end up tests and comments on the key aspects of the code changed (not 100pct, just things very important or likely to break on upgrade). So, this is about how much work you have, as me pulling and building for review isn't so much effort. I can already tell we are on the same page with the eventual outcome.
Let's say we eventually wanted the triple horizontal line pull down (option 3), well we could start with moving both (lang, theme) to the far right (option 2), then later move both into a triple horizontal pull down (option 3) without a lot of distraction to the user provided the defaulting and browser storage was the same. Regardless, agree we can compress to language code. If you want to do the language code compression in a separate PR you can, also.
Over to you, and thanks for so much the thinking, as much as the working, on this! It is a lot easier for me to help review when folks do diligence as you have!
fyi I will be away for a while starting saturday, so if follow-up are needed will need a different contributor to progress.
I implemented option 2, creating a button that changes the selected theme upon clicking. It uses two icons to identify, in this case, the current theme. There was also the option for the button to display the icon of the theme that would be applied upon clicking. Personally, I don't think there's any difference between the two approaches; both are used. The React website (https://react.dev/) uses the second option described, while the site of Docusaurus, for example, uses the one I implemented (https://docusaurus.io/).
Let me know what you think, also regarding the position. If you want, we can open a dedicated issue for applying the ISO 639-1 format for the language, especially since I noticed that some texts are missing for French and Chinese, I don't know if it's intentional ("Run Query" for example).
If it's okay, I will soon start thinking about some tests that can be implemented.
Thanks for the progress. So, about positioning, I meant to put both controls to the right of the search bar because as you noticed, things start to feel more odd when they are "sandwiched" together. It is a mix of UI controls and actual functionality. Notably, the "upload trace" which is an alternative to choosing one, is between UI controls now.
I do like the icon.. you chose.
For ISO 639-1, I think let's limit it simply to shortening to keep your work down especially as I'm not likely to do a lot of PR review when on vacation. So, use the 2 letter code instead of long form. You can do that in a PR to merge before this one, then rebase this, or you can do it here. In any case, I think merging this without that done adds weight to the top bar, especially for folks like me who use low resolution due to "old man eyes" ;)
Here, I think the main change is to move both UI controls and to the right of the vertical line. When we release this code, we can ask for feedback. As you notice, no one else is giving feedback, so we have to do our best then let community tell us if they didn't like it.
On the glitches in translations, I'm not entirely surprised. If you want to do a PR to fix some of the gaps, please do.
Cheers!
Sorry for writing only now. I have made changes based on what we discussed. This is the final result. I preferred to directly push the changes to the ISO 639-1 language selection and also a couple of missing translations.
And here's the before and after. The top-bar controls use about the same space as they did before, and better organized IMHO
before:
after:
great work @giaroc, I tried my best to summarize the points in the commit message https://github.com/openzipkin/zipkin/commit/03c9588110afdfd1e917db33c1b2ddaa54c8d073 (this org squashes always, so it is one commit on merge)
Issue: #3537
Enhancements for Accessibility:
Error Trace Symbol: Added a distinct symbol next to traces with errors for immediate recognition without color dependency.
Duration Readability: Adjusted duration text placement to outside the colored bars, improving readability by avoiding text and color overlap.
Color Choice Optimization: Revised color palette for the page to enhance contrast and ease of distinction for users with color blindness.
Custom Theme Support:
Introduced options for users to select from three themes for page display: Light Theme (default), Dark Theme, and High Contrast Theme. Aimed at enhancing user experience by providing customization options based on individual preferences.
Changes Summary:
Reasoning: These changes aim to make the "Find a Trace" page more accessible and user-friendly for all users, including those with visual impairments.