Skip to content
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

Realign accessible color logic around new env var #185

Open
wants to merge 3 commits into
base: accessible-colors
Choose a base branch
from

Conversation

andyfeller
Copy link
Member

@andyfeller andyfeller commented Mar 6, 2025

Relates github/cli#830

  1. Updates existing accessibility detection logic specifically around accessible colors
  2. Updates to testing regarding changes
  3. Various other enhancements to related code and comments

1. Introduce constant for `gh config` setting for accessible colors
2. Align existing constant for env var to match
3. Rename exported functions to distinguish use specifically for color
4. Minor enhancements to comments and tests
@williammartin
Copy link
Member

Does this really "fix https://github.com/github/cli/issues/830"? Maybe it won't close the issue because it's cross org but it seems to me we need to either merge this to main and release or point cli/cli to this commit right? Is there any other work required beyond that?

@andyfeller
Copy link
Member Author

Does this really "fix github/cli#830"?

No, good call out; I've updated this to "relates" because your next comment is on point: we need to start using it ...

Maybe it won't close the issue because it's cross org but it seems to me we need to either merge this to main and release or point cli/cli to this commit right? Is there any other work required beyond that?

... I think we have potentially 3 choices of how to move forward:

  1. Merge this into default branch and release it

    If we did this now as-is, then any markdown package user who wanted to update cli/go-gh would be required to do a light/moderate bit of heavy lifting to switch over to cli/glamour.

    They will also have to undo it later when we finally contribute our fork changes upstream and remove cli/glamour; not a great experience.

  2. Point cli/cli to this commit

    This would allow us begin using it without affecting other cli/go-gh users.

    It also means that we have to be vigilant about other cli/go-gh changes going into the default branch and keeping accessible-colors up to date until we can get the upstream changes accepted.

  3. Move accessible-colors work under x/markdown package separate of markdown

    Your comment about about marking or signaling that functions are experimental reminded me of golang.org/x of charmbracelet/x.

    The idea being that this could:

    1. Merge this into the default branch and release
    2. Avoid impacting cli/go-gh users of markdown
    3. Reduce some of our toil iterating on cli/cli and cli/go-gh while getting changes accepted upstream
    4. Communicate status of these new changes, maybe even help to get feedback from those interested who pick up on them

@andyfeller
Copy link
Member Author

andyfeller commented Mar 7, 2025

Post-discussion with Will about the above points

  1. We think cli/glamour changes should go into upstream before merging accessible-colors to avoid disrupting GitHub CLI extension authors

  2. We think expanding color capabilities for GitHub CLI and CLI is at odds with accessibility package and want to do the following

    • Rename the accessibility package to color
  3. We want to ensure these changes are understood to be experimental and will do the following

    • Add in-line comments about the experimental status of the accessible color logic
    • Move the color package to x/color to communicate experimental status

Reasoning

When our efforts to build a more accessible GitHub CLI experience started, we didn't know the full scope of how it would play out in cli/cli and cli/go-gh changes. We knew there were concerns around colors in general, colors in markdown, and screen readers working with prompting. As we have made progress, the accessibility package feels the wrong way to organize this new behavior versus what it is affecting.

The question is where does this live?

  • config: underlying gh config and guts of GitHub CLI configuration
  • prompter: interactive user prompting
  • term: terminal environment and capability handling

The idea of a new color package was motivating bringing GitHub CLI color handling logic out of cli/cli and into cli/go-gh while expanding on ideas from Primer Design System and color roles.

Following Go `x` convention seen across various projects, the former `accessibility` package has been relocated to communicate its experimental status more clearly to extension authors.

It has also been renamed to `color` to align with adopting Primer Style color role functionality.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants