-
Notifications
You must be signed in to change notification settings - Fork 7
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
Move CSS properties closer to their components and add dark mode #17
base: master
Are you sure you want to change the base?
Conversation
Added a dark mode implementation. Could be a separate PR in theory so please let me know if I should split this up, but it's just a little bit of code. |
01c81e9
to
c029a4e
Compare
Nice! I'm looking forward to trying this out, but it might be a few days before I can get to it. |
Sounds good. I know I'm just kind of Kool Aid Man-ing my way into your project here and I appreciate your comments so far. I always thought it would be fun to work on the wiki but didn't want to do all the annoying parts...which it seems you've already done. No rush. |
Took dark mode for a test drive. First impression is that it looks pretty decent, but I have some suggestions.
If there is an overarching philosophy to how I built this whole project, it would be: I never add something until I need it. It's always sooo tempting to say, "oh yeah, I'll need that five commits down the road," or, "hey that looks easy and fun to implement so I might as well do it," or even occasionally "lots of people do it this way, it must be for a good reason I just don't understand yet." This way lies madness and technical debt, ask me how I know. :) So I extended that philosophy to the style sheets. I tried not to add classes and IDs everywhere unless they were absolutely necessary to avoid repetition. That said, I'm not an expert at CSS or UI design in general. I just know what I like. So if the CSS can be simplified by adding classes, by all means, feel free. Now on to the dark mode... I don't plan on using dark mode (much?) so arguably I don't have a horse in the race. So take everything past this as suggestions that won't necessarily block the merge. The main suggestion I would make is that the main content area has quite a lot of contrast. Looks like bright white on dark black to my eyes but I didn't check the actual colors. I wonder if switching the side panel and content area backgrounds would help? It's something I can play around with, maybe on the weekend. I didn't try the textarea editor, but for the CodeMirror editor, I just ripped the CSS straight out of Pygments. Probably the default theme. I expect we can do the same for the dark mode editor. In fact, they have some lovely dark mode palettes that might be worth looking at for the rest of the CSS as well. Just a thought. |
Less indirection between components and their colors. If a variable is only used once, inline it.
Use these fancy new design tokens to implement dark mode. Colors are very roughly chosen.
c029a4e
to
d88309c
Compare
Couple of changes:
I did not add a dark theme for CodeMirror; see #15 (comment) |
Less indirection between components and their colors. If a variable is only used once, inline it. Also, my autoformatter made some changes that I can revert if necessary, but it sure is convenient to have the syntax checked all the time.
I'm learning more about the structure of the CSS as I go. It seems to lean heavily on the way the HTML is structured, which is fine up to a point, but I suspect it could be simplified a lot by using more classes. I'll work on that and see how it goes.