Add accessible themes

[jtmcg]

Changes

• Add accessibility file to markdown package:
  • Adds ansi color code enum
  • Adds accessible glamour themes
• Add check for accessibility to WithTheme
• Add tests to cover rendering outcomes

Notes

The link colors weren't rendering correctly with the provided markdown colors. In particular, it seemed that the link color was overriding the link text color. As such, I've changed the link stylings so that the text renders in either the bright blue or bright cyan.

Testing

1. Pull down this branch
2. In the cli/cli repo, add a replace directive pointing at your local version of go-gh: go mod edit -replace=github.com/cli/go-gh/v2={YOUR_LOCAL_PATH}/go-gh
3. Build the binary
4. Run ACCESSIBLE=true ./bin/gh repo view

Screenshots

Dark Theme

<= current | accessible =>

Light Theme

<= current | accessible =>

[andyfeller]

@jtmcg : Really like what I'm seeing especially the tests! Nothing majorly blocking, some curious questions for your thoughts on various topics.

[andyfeller]

No right or wrong answer, but I take it you think we should selectively override the colors versus duplicating the whole style and maintaining it separately?

[jtmcg]

The side-effects of overwriting the glamour styles were giving me pause... Mostly, I think this makes following the logic of what's happening for the accessibility experience easier to follow. After we chatted the other day about "how will we know if we're light or dark?" and the answer being "let glamour decide" I thought it more helpful to put the accessible things where that decision happens and be explicit about how its happening.

Compare my approach:

go-gh/pkg/markdown/markdown.go

Lines 35 to 50 in ac8e455

	func WithTheme(theme string) glamour.TermRendererOption {
	style := os.Getenv("GLAMOUR_STYLE")
	accessible := accessibility.IsEnabled()
	if style == "" || style == "auto" {
	switch theme {
	case "light", "dark":
	if accessible {
	return glamour.WithStyles(AccessibleStyleConfig(theme))
	}
	style = theme
	default:
	style = "notty"
	}
	}
	return glamour.WithStylePath(style)
	}

To yours

go-gh/pkg/markdown/markdown.go

Lines 54 to 68 in 0c30d0f

	func Render(text string, opts ...glamour.TermRendererOption) (string, error) {
	if accessible := os.Getenv("ACCESSIBLE"); accessible != "" {
	makeDefaultStyleColorsAccessible()
	}
	// Glamour rendering preserves carriage return characters in code blocks, but
	// we need to ensure that no such characters are present in the output.
	text = strings.ReplaceAll(text, "\r\n", "\n")
	opts = append(opts, glamour.WithEmoji(), glamour.WithPreservedNewLines())
	tr, err := glamour.NewTermRenderer(opts...)
	if err != nil {
	return "", err
	}
	return tr.Render(text)
	}

I find the logic of "this is how we are choosing to use the accessible light/dark theme" much easier to understand in the former vs the latter.

What are your thoughts?

[andyfeller]

The side-effects of overwriting the glamour styles were giving me pause... Mostly, I think this makes following the logic of what's happening for the accessibility experience easier to follow.

Sorry, let me clarify.

I understand why these changes only override the styles if WithTheme() is called as someone using markdown.Render() without providing the TermRenderer will not get any color.

My question was primarily focused on what value we'd get by statically duplicating DarkStyleConfig and LightStyleConfig variables in cli/go-gh over run-time copying and overriding specific fields.

Not something that needs to be done; just curious for your thoughts. I can imagine this might make sense in the future as we increase the scope of customizing the styles including code block rendering, modifiers, and/or changing the resulting format of certain elements.

[jtmcg]

Oh, I understand, like why not "copy/paste" the darkStyleConfig from glamour into go-gh, rename for accessibility, and use that instead of creating this at run-time?

There's two benefits to this, I think:

1. This makes it really clear how we're deviating from Glamour's styles for accessibility
2. Given we are talking about contributing back to Glamour for this, the less we have to maintain here the better.

[andyfeller]

Only 1 minor change to fix and let's ship it!

I'm leaving comments regarding a few errant thoughts on other aspects but nothing that's blocking for now. I look forward to seeing how our testing around style customization evolves.

[andyfeller]

What are your thoughts if we are capable of building a more comprehensive test that can traverse ansi.StyleConfig type for any colors we did not override to ensure they are customizable?

I realize that will be significant enough work to be a separate issue and PR, but want to gather your thoughts while we're here.

[jtmcg]

While it would be an interesting engineering problem, I don't think its one we should solve until we have a reason to, i.e. we screw something up and this suggestion would keep us from doing it again.