feat(charm): add lipgloss styling with Slack brand colors under charm experiment#365
feat(charm): add lipgloss styling with Slack brand colors under charm experiment#365
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #365 +/- ##
==========================================
+ Coverage 65.19% 65.49% +0.29%
==========================================
Files 218 218
Lines 17981 18120 +139
==========================================
+ Hits 11723 11867 +144
+ Misses 5164 5160 -4
+ Partials 1094 1093 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
@srtaalej Thanks for sharing these changes! I'm approving now in moon light so we might want to follow up with adjustments to the theme next 🌚
Some of these comments suggest that this PR should be broken up into perhaps multiple separate changes. No blocker since I think it's working alright but review might be more focused I hope - perhaps as: 🙏
- chore(deps): update charm packages to v2
- refactor: use style package for all stylings
- feat(experimental): update branding and styles
- feat(experimental): format help page outputs
Both 1 and 2 LGTM but I want to leave a few comments toward 3 then 4 next:
With a few test runs using the experiment I find that:
- Errors are shown with green outputs which are confusing to skim I felt 👾
- Section headings aren't contrasted with section text which make walls of text
- Debug outputs lose highlighting after the experiment becomes active 🐛
Some related writings I find kind are clig.dev but we might find compromise in amount of formats for preference to theme. FWIW I find colors of the help pages helpful after patterns are gathered! Toward 4 we might want to consider the fang package for included batteries? 🔋 ⚡
Let's get this merged I think! The above can be discussed separate but needn't block experiment 🚢
| // Navigate down to "View more samples" (4th option, index 3) | ||
| _, cmd := f.Update(tea.KeyMsg{Type: tea.KeyDown}) | ||
| _, cmd := f.Update(tea.KeyPressMsg{Code: tea.KeyDown}) | ||
| doAllUpdates(f, cmd) | ||
| _, cmd = f.Update(tea.KeyMsg{Type: tea.KeyDown}) | ||
| _, cmd = f.Update(tea.KeyPressMsg{Code: tea.KeyDown}) | ||
| doAllUpdates(f, cmd) | ||
| _, cmd = f.Update(tea.KeyMsg{Type: tea.KeyDown}) | ||
| _, cmd = f.Update(tea.KeyPressMsg{Code: tea.KeyDown}) | ||
| doAllUpdates(f, cmd) | ||
| _, cmd = f.Update(tea.KeyMsg{Type: tea.KeyEnter}) | ||
| _, cmd = f.Update(tea.KeyPressMsg{Code: tea.KeyEnter}) |
There was a problem hiding this comment.
🪬 thought: These changes aren't related to lipgloss which make review evermore difficult but also make me think we're testing too much related to prompts here. I'm not familiar with this implementation but I'd expect assertions that expected values are passed to huh forms?
There was a problem hiding this comment.
these were updated to match the new v2 syntax for key presses - i agree this PR got really big really fast. i wanted to update to v2 here to resolve the bugs we had with the huh forms which i felt were related to styling. i just didnt realize there would be so much needing changing 😭
| // ════════════════════════════════════════════════════════════════════════════════ | ||
| // DEPRECATED: Legacy help template — aurora styling | ||
| // | ||
| // Delete this entire block when the charm experiment is permanently enabled. | ||
| // ════════════════════════════════════════════════════════════════════════════════ | ||
|
|
||
| const legacyHelpTemplate string = `{{.Long}} |
There was a problem hiding this comment.
🔍 suggestion: We should avoid changing existing code with experimental changes. I imagine we'll want to rename charmHelpTemplate back to helpTemplate after this experiment concludes, but references to stable code is being changed often during this. I'm not against moving code but it adds more to review!
| "non-breaking warning continues without prompt": { | ||
| warnings: slackerror.Warnings{ | ||
| {Code: "some_warning", Message: "just a warning"}, | ||
| }, | ||
| expectedResult: true, | ||
| }, |
There was a problem hiding this comment.
🧪 suggestion: While we're adding this we should confirm clients.IO.ConfirmPrompt isn't called in asserts below to match the test case. This might otherwise be prompting a default "no" to confirming changes of the manifest-
| span, _ := opentracing.StartSpanFromContext(ctx, "printInfo", opentracing.Tag{Key: "printInfo", Value: message}) | ||
| defer span.Finish() | ||
| } | ||
| io.Stdout.Println(style.Styler().Reset(message)) |
There was a problem hiding this comment.
👁️🗨️ issue: I'm not super confident in this change to production code since this might've been guarding "info" output from being formatted in terminals as bold or secondary from previous lines.
We might continue to support this with exception for the charm experiment? I agree that it's not ideal to support edge cases of earlier output but it might keep experiences most stable.
Co-authored-by: Eden Zimbelman <eden.zimbelman@salesforce.com>
Co-authored-by: Eden Zimbelman <eden.zimbelman@salesforce.com>
Changelog
Adds lipgloss-based styling to all
helpoutput under the charm experiment flag, using a centralized Slack brand color palette shared across all CLI components, and migrates all direct Styler() usage to named style functions.Summary
This PR started as a focused effort to add lipgloss styling to help output, but grew in scope for two main reasons:
huhto v2 was necessary to fix bugs in the interactive forms related to styling. The v2 API introduced breaking changes to key press handling in tests (create_template_charm_test.go), requiring updates across all huh form test files.Styler()call sites were migrated to named style functions (Green(),Red(),Yellow(),Gray()). This touched many files but each change is mechanical.The PR covers four logical areas:
chore(deps): Bump charm packages (huh, lipgloss) to v2refactor: Migrate allStyler()call sites to named style functionsfeat(experimental): Add Slack brand color palette (colors.go) and lipgloss style implementationsfeat(experimental): Add charm-styled help page templates with flag colorizationScreenshots
Test steps
Known issues (follow-up)
Per reviewer feedback, these will be addressed in follow-up PRs:
Code changes (to keep this PR from growing further):
IsCharmEnabled()frominternal/styleand useclients.Config.WithExperimentOn(experiment.Charm)checks outside the style package instead(
help.go:73,style.go:63). Deferred because it also requires changes tocmd/platform/run.go.charmHelpTemplateback to
helpTemplatewhen the experiment concludes (help.go:127)create_template_charm_test.goto assert expected values passed tohuhformsrather than testing prompt interactions directly
Requirements