-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Color tweaks for Error Handling. #2676
Color tweaks for Error Handling. #2676
Conversation
…color schemes to top of definition.
…s for light theme.
Tweak away. We'll comment if we think you've gone too far. I much appreciate this and we can probably work to reduce the !important by bringing some changes to tracerite. At first glance, my biggest concern though is the need for more branding colors. Mainly, the background color differs from the docs background color in dark mode, and I don't see the Sanic color represented. |
sanic/pages/styles/BasePage.css
Outdated
|
||
@media (prefers-color-scheme: dark) { | ||
:root { | ||
--sanic: #ef4444; |
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.
this value is meant to be Sanic's on brand color. It should not differ in dark mode.
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.
this value is meant to be Sanic's on brand color. It should not differ in dark mode.
Sure I can fix this.
Sure. Right now the Sanic brand is only a brand element. It’s possible to just swap it for places where there are reds. I think it depends on how strongly you wish to see the Sanic Magenta. For example:
I was under the impression that I have to keep the greens as greens and blues as blues. If that’s not the case then there are better flexibility. Looking at optionsIn terms of looking at color options, I might do some Photoshop drafts. Doing it in code is not impossible but it’s not always the best way if you’re interested in looking at options. I will think about it. |
Changes:
Background OptionsLighter. I prefer this one. It’s lower contrast. Darker. This is the exact black used in the docs. I think that it is too dark and makes reading uncomfortable. FYI I actually never read sanic docs in dark mode coz I think that the background is too dark. I know these things are super subjective. |
…verride. Add darker sanic background (111) as option but in the comments so we can switch back if need be.
…y changed and nudged.
This is now updated with all the latest. |
Okay, thanks for the work. I have to say I didn't have time to read the design discussion you had in full, but this looks very colorful to me, a lot of bright colors. This comment is of the screenshots above. In case you didn't update them after changes, please post new ones (you can enforce light mode under Chrome developer tools renderer panel if you normally use dark mode, to have screenshots of both). I kind of prefer bringing emphasis to the relevant details by not making everything bright. The combination of mostly white and grey accomplished this, making the exception stand up in Sanic red (brand color) and highlighting the relevant code lines with the yellow marking. A little bit of color in variable inspectors makes them visually distinct of code, but not all those three parts of it need to be colored. I initially tried passive and similar colors (greenish-blueish) to keep it distinct but not grab too much attention, not appear like a rainbow. These are of course also matters of opinion, and perhaps you have discussed this, but just my points at this phase. Also, some of the styling changes should ideally be done on Tracerite, with only those specifically needed for Sanic left to be done on Sanic side (especially the use of that Sanic red). But bear in mind that the module also needs to work on Jupyter notebooks where themes come in all colors (but don't use prefers-color-scheme, unfortunately), and the module is purposefully made to use default text and background colors in the majority of things (gray text is a fairly safe choice that stands out from either dark or light backgrounds). |
--sanic-tab-background: #484848; | ||
--sanic-tab-text: #e1e1e1; | ||
--sanic-tab-shadow: #000; | ||
--sanic-highlight-background: var(--sanic-yellow); |
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.
The yellow can be a lot darker (and less disruptive) on dark mode. Yellow is a strange color in that it only seems yellow if it is brighter or similar brightness to its surroundings, which is why on black background it can be quite dark while on white background it needs to be close to #ff0. Yellow (or orange) darker than its surroundings is seen as (greenish) brown, a different color and not simply a darker shade.
Color SchemeJust saw this — I’d like to let you know that I have reverted many of the color recommendations that I’ve made, because based on discussion with Adam, it’s clear that he prefers specific colors. I myself prefer dark modes to not have so much contrast, and prefer low-contrast color scheme in dark mode. Using a real life example that you can see, on Twitter, I use the dimmed mode, which is a low contrast white on tinted blue gray background. Twitter also offers a white text on black background option which I find to have too much contrast. It’s the same for me in terminals, and the editors that I use. I would never use near-black for background for these reasons. Given these are highly subjective, and that Adam felt that these color discussion were taking too long, I yield to his wishes and implemented the colors he posted on Discord. The screenshots are as follows: Changes in traceriteI absolutely agree with you that some of these changes probably should be done in tracerite. I thought that I had mentioned this on Discord already but perhaps I didn’t make myself understood very well. I’d be happy to make the necessary changes in tracerite. For Tracerite I think that we should handle some of the minor typographic things, and maybe add a few more variables for colors so that people can override them more easily. PingSomehow I didn’t get a notification for these comments so I just saw these now. Sorry about that. Feel free to ping me on Discord anytime as I’m on Discord 24/7 for work. |
Regarding this, I am open to it. I just don’t know exactly what is preferred. I feel like that we should probably chat about this. Colors and palettes are inherently subjective. I’d be happy to make a few tests in Photoshop and discuss options. Sometimes with things related to palettes, doing it in code directly is not the best approach when there are multiple stakeholders. It’s also something that takes notoriously long to resolve — “can we make this 1% darker? 1% lighter?” I kid you not but I’ve once spent a month tweaking a single color for a client. your perception of colors also change depending on your mood and time of the day. Are you looking at the dark mode during the day or at night? True perception of color is not just what’s on the screen but your environment plays a huge role. All of this makes it harder to come to a consensus. I guess the primary discussion point here is:
|
I suggest doing experiments in browser devtools with #rgb (short) color codes, but whatever works for you :D Looks like overall we are pretty much on the same page regarding colors. The brand color and not touching overall background color seem important, other than that things are quite flexible. My preference is maybe for even slightly less colors still but that is definitely not a strong opinion on my part. I believe we found a reasonable solution to the code font as well, by using anything by Courier New. The less bright yellow on dark mode should go well with your desire for them to be less bright, although normal text would stay whatever shade the page sets - but IMO Sanic's base page can use #eee or #ddd for that on dark mode (not sure what it currently uses). If values in variable inspectors are not colored, they should either be grey Feel free to experiment and post new screenshots if you want opinions. And you are right, this can definitely turn into massive bikeshedding, that we really ought to avoid. |
FWIW I put some of my own tweeks of your PR (admittedly before your last change I believe) into this branch: https://github.com/sanic-org/sanic/tree/sml-changes. I definitely do not want to bikeshed on colors and spend a month bumping a color 1% or 10%. If this means dropping all colors except for background, foreground, and Sanic-accent: fine. Let's get the feature done and we can iterate on it. But I am hoping to not let colors be what holds back the PR getting merged. |
I've been using tracerite 1.0.1 on VSCode Jupyter, Jupyter Notebook and Jupyterlab, seems to work great in all of those with its current styles. |
@smlbiobot Are you doing further changes to this or should we merge now? |
@Tronic I’m not doing anything more to this unless there is something specific that you want me to do. As explained in previous comments, Adam seems set on specific colors so I won’t make any changes to them for now. We can discuss further colors e.g. making them more subtle on Discord. I don’t think that it makes sense to do color options on the code directly like this. We need to first agree if we want to go with full color options or shades with gray with magenta accents. Then we can look at some option. Adjusting colors back and forth like this won't be productive. |
I agree. Let's get this done and then come back to coloring. I like the idea of maybe just handling it with various shades. I suggest we continue the coloring discussion and come back before release. My only question though is do we need a corresponding PR into |
What’s done:
:root {}
and the dark theme config immediately follows. For me this makes color management easily to deal with.CSS for the error page
I had to add some
!important
styles. But as mentioned on Discord, it would be better if the container on the page would have an ID so that CSS can just use#sanic-error
to take precedence over the styles fromtracerite
.Right now, these
!important
CSS values are mainly used to override the styles fromtracerite
.Light Theme Screenshot
Dark Theme Screenshot
Further Notes
I’m not sure how far I am allowed to tweak these so I kept it to colors only and some minor typographic issues.