Add accessible themes #182

jtmcg · 2025-02-12T00:08:00Z

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

Pull down this branch
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
Build the binary
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.

pkg/markdown/markdown_test.go

pkg/markdown/accessibility.go

andyfeller · 2025-02-12T17:33:43Z

pkg/markdown/accessibility.go

+func accessibleDarkStyleConfig() ansi.StyleConfig {
+	cfg := glamour.DarkStyleConfig
+
+	// Text color
+	cfg.Document.StylePrimitive.Color = White.ValuePtr()
+
+	// Link colors
+	cfg.Link.Color = Black.ValuePtr()
+	cfg.LinkText.Color = BrightCyan.ValuePtr()
+
+	// Heading colors
+	cfg.Heading.StylePrimitive.Color = BrightMagenta.ValuePtr()
+	cfg.H1.StylePrimitive.Color = BrightWhite.ValuePtr()
+	cfg.H1.StylePrimitive.BackgroundColor = BrightBlue.ValuePtr()
+
+	// Code colors
+	cfg.Code.BackgroundColor = BrightWhite.ValuePtr()
+	cfg.Code.Color = Red.ValuePtr()
+
+	// Image colors
+	cfg.Image.Color = BrightMagenta.ValuePtr()
+
+	// Horizontal rule colors
+	cfg.HorizontalRule.Color = White.ValuePtr()
+
+	return cfg
+}


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?

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?

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.

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:

This makes it really clear how we're deviating from Glamour's styles for accessibility

Given we are talking about contributing back to Glamour for this, the less we have to maintain here the better.

pkg/accessibility/accessibility.go

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.

pkg/markdown/accessibility_test.go

andyfeller · 2025-02-14T16:05:16Z

pkg/markdown/accessibility_test.go

+func Test_accessibleDarkStyleConfig(t *testing.T) {
+	cfg := accessibleDarkStyleConfig()
+	assert.Equal(t, White.StrPtr(), cfg.Document.StylePrimitive.Color)
+	assert.Equal(t, BrightCyan.StrPtr(), cfg.Link.Color)
+	assert.Equal(t, BrightMagenta.StrPtr(), cfg.Heading.StylePrimitive.Color)
+	assert.Equal(t, BrightWhite.StrPtr(), cfg.H1.StylePrimitive.Color)
+	assert.Equal(t, BrightBlue.StrPtr(), cfg.H1.StylePrimitive.BackgroundColor)
+	assert.Equal(t, BrightWhite.StrPtr(), cfg.Code.BackgroundColor)
+	assert.Equal(t, Red.StrPtr(), cfg.Code.Color)
+	assert.Equal(t, BrightMagenta.StrPtr(), cfg.Image.Color)
+	assert.Equal(t, White.StrPtr(), cfg.HorizontalRule.Color)
+
+	// Test that we haven't changed the original style
+	assert.Equal(t, styles.LightStyleConfig.H2, cfg.H2)
+}


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.

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.

pkg/accessibility/accessibility.go

jtmcg added 2 commits February 11, 2025 16:06

Add AnsiColor enum

1196ae7

Add accessible styles

897a7fe

jtmcg force-pushed the jtmcg/add-accessible-themes branch from 4ed71e2 to 9ccc524 Compare February 12, 2025 00:11

jtmcg changed the base branch from trunk to accessible-colors February 12, 2025 00:11

jtmcg force-pushed the jtmcg/add-accessible-themes branch from 9ccc524 to ac8e455 Compare February 12, 2025 00:12

andyfeller reviewed Feb 12, 2025

View reviewed changes

jtmcg force-pushed the jtmcg/add-accessible-themes branch 2 times, most recently from cc13bb1 to 7f030f6 Compare February 12, 2025 23:14

jtmcg added 6 commits February 12, 2025 15:26

Add accessible themes to markdown rendering

d2d3fcf

Cleaned up naming and comments for better understandability

dddc61a

Upgrade glamour version for access to styles package

5bd4f3e

Fix typo in env var name

6713a5d

Change link stylings so link text is correct color

2ef01cb

go mod tidy

b76f1c0

jtmcg force-pushed the jtmcg/add-accessible-themes branch from 7f030f6 to b76f1c0 Compare February 12, 2025 23:26

jtmcg marked this pull request as ready for review February 12, 2025 23:30

jtmcg changed the title ~~Jtmcg/add accessible themes~~ Add accessible themes Feb 12, 2025

jtmcg commented Feb 13, 2025

View reviewed changes

pkg/accessibility/accessibility.go Show resolved Hide resolved

andyfeller requested changes Feb 14, 2025

View reviewed changes

jtmcg added 2 commits February 14, 2025 10:08

Update test to check correct config

bb3c0d2

Fix typo

06206cc

andyfeller approved these changes Feb 14, 2025

View reviewed changes

jtmcg merged commit 607ac5b into accessible-colors Feb 14, 2025
8 checks passed

jtmcg deleted the jtmcg/add-accessible-themes branch February 14, 2025 19:41

andyfeller mentioned this pull request Mar 11, 2025

Introduce experimental accessible colors in markdown rendering #186

Merged

	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)
	}

	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)
	}

Add accessible themes #182

Add accessible themes #182

Uh oh!

Conversation

jtmcg commented Feb 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Notes

Testing

Screenshots

Dark Theme

Light Theme

Uh oh!

andyfeller left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

andyfeller Feb 12, 2025

Choose a reason for hiding this comment

Uh oh!

jtmcg Feb 12, 2025

Choose a reason for hiding this comment

Uh oh!

andyfeller Feb 14, 2025

Choose a reason for hiding this comment

Uh oh!

jtmcg Feb 14, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

andyfeller left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

andyfeller Feb 14, 2025

Choose a reason for hiding this comment

Uh oh!

jtmcg Feb 14, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

jtmcg commented Feb 12, 2025 •

edited

Loading