Skip to content

Warn on opaque needsTransparency colors, expose to workbench#200052

Merged
Tyriar merged 3 commits intomainfrom
tyriar/200050
Dec 5, 2023
Merged

Warn on opaque needsTransparency colors, expose to workbench#200052
Tyriar merged 3 commits intomainfrom
tyriar/200050

Conversation

@Tyriar
Copy link
Copy Markdown
Contributor

@Tyriar Tyriar commented Dec 5, 2023

fixes #200050

The relatively complex regex matches #000e, #000000fe but not #000f, #000000ff

image
image

@Tyriar Tyriar added this to the December 2023 milestone Dec 5, 2023
@Tyriar Tyriar requested a review from aeschli December 5, 2023 15:18
@Tyriar Tyriar self-assigned this Dec 5, 2023
@Tyriar Tyriar enabled auto-merge December 5, 2023 15:19
* @description the description
*/
registerColor(id: string, defaults: ColorDefaults, description: string): ColorIdentifier;
registerColor(id: string, defaults: ColorDefaults, description: string, needsTransparency?: boolean): ColorIdentifier;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't expect an option boolean parameter to registerColor to control linting settings, so I'd make it an option:

Suggested change
registerColor(id: string, defaults: ColorDefaults, description: string, needsTransparency?: boolean): ColorIdentifier;
registerColor(id: string, defaults: ColorDefaults, description: string, options?: { needsTransparency?: boolean }): ColorIdentifier;

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aeschli shall I do this consider we're exposing it on the registry interface now?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change is fine, but not really needed. ColorRegistry.registerColor already has that parameter and it is used at several places. @hediet I can make the suggested change when we get more arguments

* @description the description
*/
registerColor(id: string, defaults: ColorDefaults, description: string): ColorIdentifier;
registerColor(id: string, defaults: ColorDefaults, description: string, needsTransparency?: boolean): ColorIdentifier;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change is fine, but not really needed. ColorRegistry.registerColor already has that parameter and it is used at several places. @hediet I can make the suggested change when we get more arguments

@Tyriar Tyriar merged commit 85b1ab9 into main Dec 5, 2023
@Tyriar Tyriar deleted the tyriar/200050 branch December 5, 2023 15:36
@github-actions github-actions bot locked and limited conversation to collaborators Jan 19, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Warn in themes when opaque colors would hide selection

3 participants