-
Notifications
You must be signed in to change notification settings - Fork 54
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
Ensure codeblocks use accessible colors #183
base: accessible-colors
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,6 +7,7 @@ import ( | |
"strings" | ||
"testing" | ||
|
||
"github.com/MakeNowJust/heredoc" | ||
"github.com/cli/go-gh/v2/pkg/accessibility" | ||
"github.com/stretchr/testify/assert" | ||
"github.com/stretchr/testify/require" | ||
|
@@ -74,6 +75,48 @@ func Test_Render(t *testing.T) { | |
accessibleEnvVar: "true", | ||
wantOut: fmt.Sprintf("%s1mh2", brightMagenta_4bitColorSeq), | ||
}, | ||
{ | ||
name: "when the light theme is selected, the codeblock renders using 8-bit colors", | ||
text: heredoc.Docf(` | ||
%[1]s%[1]s%[1]sgo | ||
fmt.Println("Hello, world!") | ||
%[1]s%[1]s%[1]s | ||
`, "`"), | ||
theme: "light", | ||
wantOut: "\x1b[0m\x1b[38;5;235mfmt\x1b[0m\x1b[38;5;210m.\x1b[0m\x1b[38;5;35mPrintln\x1b[0m\x1b[38;5;210m(\x1b[0m\x1b[38;5;95m\"Hello, world!\"\x1b[0m\x1b[38;5;210m)\x1b[0m", | ||
}, | ||
{ | ||
name: "when the dark theme is selected, the codeblock renders using 8-bit colors", | ||
text: heredoc.Docf(` | ||
%[1]s%[1]s%[1]sgo | ||
fmt.Println("Hello, world!") | ||
%[1]s%[1]s%[1]s | ||
`, "`"), | ||
theme: "dark", | ||
wantOut: "\x1b[0m\x1b[38;5;235mfmt\x1b[0m\x1b[38;5;210m.\x1b[0m\x1b[38;5;35mPrintln\x1b[0m\x1b[38;5;210m(\x1b[0m\x1b[38;5;95m\"Hello, world!\"\x1b[0m\x1b[38;5;210m)\x1b[0m", | ||
}, | ||
{ | ||
name: "when the accessible env var is set and the light theme is selected, the codeblock renders using 4-bit colors", | ||
text: heredoc.Docf(` | ||
%[1]s%[1]s%[1]sgo | ||
fmt.Println("Hello, world!") | ||
%[1]s%[1]s%[1]s | ||
`, "`"), | ||
theme: "light", | ||
accessibleEnvVar: "true", | ||
wantOut: "\x1b[0m\x1b[30mfmt\x1b[0m\x1b[33m.\x1b[0m\x1b[36mPrintln\x1b[0m\x1b[33m(\x1b[0m\x1b[90m\"Hello, world!\"\x1b[0m\x1b[33m)\x1b[0m", | ||
}, | ||
{ | ||
name: "when the accessible env var is set and the dark theme is selected, the codeblock renders using 4-bit colors", | ||
text: heredoc.Docf(` | ||
%[1]s%[1]s%[1]sgo | ||
fmt.Println("Hello, world!") | ||
%[1]s%[1]s%[1]s | ||
`, "`"), | ||
theme: "dark", | ||
accessibleEnvVar: "true", | ||
wantOut: "\x1b[0m\x1b[30mfmt\x1b[0m\x1b[33m.\x1b[0m\x1b[36mPrintln\x1b[0m\x1b[33m(\x1b[0m\x1b[90m\"Hello, world!\"\x1b[0m\x1b[33m)\x1b[0m", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I find it really hard to tell what's different between this blob of output versus the one on line 96. That's why I added the human-grokkable format in the tests above these. I also find it a bit jarring that the output formats are different for these tests and would find value in their consistency. Perhaps codeblocks are different enough to justify it, but there may be value in pulling out a human-grokkable abstraction in these There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Completely get that 💯 as that was a consideration when I was writing the tests I choose to defer to seeing if someone would comment on. 👍 I can play around to see what this would look like for a more grokkable format but I suspect the extent of control characters is pretty significant. I don't know how much more grokkable it will be trying to reuse a bunch of placeholders but we can try. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here's an example of what this would look like if we used the same approach of constants: reset = "\x1b[0m"
glamourLightCodeBlock_8bitRaisinBlackColorSeq = "\x1b[38;5;235m"
glamourLightCodeBlock_8bitVividTangerineColorSeq = "\x1b[38;5;210m"
glamourLightCodeBlock_8bitJadeColorSeq = "\x1b[38;5;35m"
glamourLightCodeBlock_8bitCopperRoseColorSeq = "\x1b[38;5;95m" {
name: "when the light theme is selected, the codeblock renders using 8-bit colors",
text: heredoc.Docf(`
%[1]s%[1]s%[1]sgo
fmt.Println("Hello, world!")
%[1]s%[1]s%[1]s
`, "`"),
theme: "light",
// wantOut: "\x1b[0m\x1b[38;5;235mfmt\x1b[0m\x1b[38;5;210m.\x1b[0m\x1b[38;5;35mPrintln\x1b[0m\x1b[38;5;210m(\x1b[0m\x1b[38;5;95m\"Hello, world!\"\x1b[0m\x1b[38;5;210m)\x1b[0m",
wantOut: fmt.Sprintf("%[1]s%[2]sfmt%[1]s%[3]s.%[1]s%[2]sPrintln%[1]s%[3]s(%[1]s%[4]s\"Hello, world!\"%[1]s%[3]s)%[1]s", reset, glamourLightCodeBlock_8bitRaisinBlackColorSeq, glamourLightCodeBlock_8bitVividTangerineColorSeq, glamourLightCodeBlock_8bitCopperRoseColorSeq),
}, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I also think there is something weird going on with the test as I originally implemented the Before introducing wantOut: "\x1b[0m\x1b[38;5;251mfmt\x1b[0m\x1b[38;5;187m.\x1b[0m\x1b[38;5;42mPrintln\x1b[0m\x1b[38;5;187m(\x1b[0m\x1b[38;5;173m\"Hello, world!\"\x1b[0m\x1b[38;5;187m)\x1b[0m", wantOut: "\x1b[0m\x1b[37mfmt\x1b[0m\x1b[37m.\x1b[0m\x1b[36mPrintln\x1b[0m\x1b[37m(\x1b[0m\x1b[33m\"Hello, world!\"\x1b[0m\x1b[37m)\x1b[0m", After adding wantOut: "\x1b[0m\x1b[38;5;235mfmt\x1b[0m\x1b[38;5;210m.\x1b[0m\x1b[38;5;35mPrintln\x1b[0m\x1b[38;5;210m(\x1b[0m\x1b[38;5;95m\"Hello, world!\"\x1b[0m\x1b[38;5;210m)\x1b[0m", wantOut: "\x1b[0m\x1b[30mfmt\x1b[0m\x1b[33m.\x1b[0m\x1b[36mPrintln\x1b[0m\x1b[33m(\x1b[0m\x1b[90m\"Hello, world!\"\x1b[0m\x1b[33m)\x1b[0m", Going to step through the debugger a bit to see what's going on. |
||
}, | ||
} | ||
for _, tt := range tests { | ||
t.Run(tt.name, func(t *testing.T) { | ||
|
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.
Clever, but I really don't like how this reads... I find it pretty confusing given this is returning a
glamour.TermRendererOption
. I guess it's a function of this call signature, but that requires implementation level knowledge of glamour to understand this code.I think I might prefer to abstract this closure out to something separate, so we end up with something like
That way, we're separating the logic of "this is how you build the accessible theme using our implementation" from "select the right theme to use"
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.
I was definitely conflicted in the implementation because of where / how we were applying the accessible theme in
WithTheme
versus inRender
where this would allow us to have separateTermRendererOption
that would be cleaner.That said, I think at some level the reader digging into this has to understand
glamour
as long as its the dependency we use here.Do you think further abstracting this could create a different type of confusion for reasoning about this behavior?