gtg922r / obsidian-numerals

An obsidian plugin which turns a math code block into a full featured calculator
Other
389 stars 7 forks source link

Fix default code block styling #35

Closed jwhitley closed 1 year ago

jwhitley commented 1 year ago

Code blocks were defaulting to --text-normal foreground text color, which broke in a light-mode theme that had dark code blocks configured. The result was zero-contrast dark text on a dark background.

One such theme is Polka, which has code block theming configurable via Style Settings, and defaults to a dark code block styling in light mode. This change ensures that Numerals' styling uses --code prefix color vars everywhere for math block styling, so it fully conforms to the active code block style.

Repro steps

  1. Create a test vault
  2. Install the Numerals plugin and the Polka theme
  3. Create a note with the sample text below.

Observe that the Numerals block is an unreadable dark-on-dark, ala:

Screenshot 2023-03-11 at 9 34 24 AM

Via the inspector, the dark text styling above is picking up the default --text-normal color from body. Instead, it should use --code-normal. This PR makes that change an catches two other instances where --text vs equivalent --code vars were used.

Result after this change:

Screenshot 2023-03-12 at 2 14 23 PM

Sample document used for repro:

# Inline code blocks

This is a color test of `inline code blocks`, code in the middle of a span of text.

# Standalone code blocks

This is a test of standalone code blocks:

wow { much code; } so => monospace


# Obsidian Numerals test

```math
lets = 40
add = 20
things = 100
up = 2
lets + add + things + up =>
jwhitley commented 1 year ago

Digging in a bit more after filing this PR, I note that there are definitely some lingering edge cases I've missed. For example, alternating line highlights aren't showing up in light mode. They appear after switching the vault to dark mode, however. I also note a styling discrepancy between the Numerals' screenshots in the README and what this PR produces. In particular, the current styling is equivalent to --text-normal for .numerals-input and --text-muted for .numerals-result. But there is no --code var which uses --text-normal by default, meaning that both input and result now use --text-muted (which is what --code-normal defaults to in Obsidian app.css). This may be solvable with a bit of color calculation, e.g. derive the result style from the input/code-normal style.

Either way, the problem above stems from Numerals' styling spanning both --text and --code values. It needs to stick in one domain or the other to ensure consistent results.

Likewise, my sample document certainly doesn't cover all of the styles this PR touches because I'm not sure how to generate those offhand in a working document. If a more comprehensive test document is available, that would be super helpful.

gtg922r commented 1 year ago

Really appreciate the PR rather than just creating an issue, especially since you clearly took the time to understand the underlying problem!

I really like the idea of switching from --text-* to --code-*. Will lead to much more consistency with themes since the text itself is in a code block.

Also adding explicit code-normal to numerals-block make sense.

You're right about code-normal changing the input text to -muted in the default theme. In Minimal and many other themes code-normal is tx1 (normal body text). The logic of matching --code seems to apply here though. If I had noticed that input text differed in color from other code blocks I probably would have changed it. Bottom line: I'm OK switching input text to code-normal which may or may not have a user impact depending on the theme.

My bigger concern is with choice of code-normal as a replacement for text-muted. I generally want there to be contrast between input and output. Minimal and many other themes move code-normal to --tx1 (body text).

I just ran some tests on many different themes and using --code-punctuation seems to work well. Its frequently mapped to --text-muted so it plays nicely. Before accepting the PR I'd prefer to use --code-punctuation as the replacement for --text-muted. Thoughts?

jwhitley commented 1 year ago

Perfect, thanks! I think —code-punctuation is likely to be a decent fit. The only real concern I have is contrast, because my experience with code themes over the years has been that they can be kinda hit-or-miss on the contrast for the non-primary-text styles. Usually comment foreground is the biggest offender. I’ll give this a spin and check contrast across a few themes. I have thoughts on a backup plan that still respects —code styling if there are problems.

gtg922r commented 1 year ago

Sounds great. I accepted the PR and pushed out a new beta release. I don't have any stats on how many people are running beta releases via BRAT, but hopefully a few who will file issues if they see any problems as well

jwhitley commented 1 year ago

Some observations:

  1. In the Default theme, --code-normal and --code-punctuation have the same value. But Minimal they're different. This is the tradeoff when using these semantic values.
  2. Checking a few themes --code-comment seems to be really problematic, at least for result. The contrast is mostly terrible at 1.85 or worse, way below the WCAG minimum of 4.0. Part of the issue (esp. in some light mode themes) is that the background is darker than the theme expects. Sometimes the starting contrast (Solarized, I'm lookin' at you) is unacceptable. But it looks like there are cases where comment contrast was OK but borderline, and the background change killed it.

All in all, I think --code-punctuation is likely to shake out as a solid choice. I have some ideas on how to both respect the code theming and give Numerals a slightly opinionated styling. I'll fiddle with those and if I get something interesting, I'll throw up a straw-man PR for further discussion.