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

consolidate Color() calls #1481

Open
EntranceJew opened this issue Mar 10, 2024 · 4 comments · Fixed by #1525
Open

consolidate Color() calls #1481

EntranceJew opened this issue Mar 10, 2024 · 4 comments · Fixed by #1525
Labels
type/enhancement Enhancement or simple change to existing functionality

Comments

@EntranceJew
Copy link
Contributor

a lot of Color() calls reinvent color values for things which already exist or are so isolated that they can't be reached and shared, so they should be consolidated

@Histalek Histalek added the type/enhancement Enhancement or simple change to existing functionality label Mar 10, 2024
@Histalek
Copy link
Member

We might want to look at our colors in general:

$ rg ' Color\(' --stats --quiet

192 matches
192 matched lines
68 files contained matches
972 files searched

We have 192 calls to Color() in 68 different files in our codebase. at a glance i can even spot a few duplicates.
Consolidating and moving the creation of these colors to a single place might be a good idea. Even if it is only to give them meaningful names.

@EntranceJew
Copy link
Contributor Author

agreed, my single biggest pain point with extending the UI was being able to reach the color variations from the VSKIN -- otherwise i could attach all the parts, i just couldn't style it to part

TimGoll added a commit that referenced this issue May 22, 2024
Unified the usage of the new spec color in some places I could find.
There are probably more, but that's it for now. Fixes #1481
@saibotk saibotk reopened this May 23, 2024
@saibotk
Copy link
Member

saibotk commented May 23, 2024

I think this needs more investigation than just the spec color :)

@TimGoll TimGoll changed the title consolidate Color() calls for spectator consolidate Color() calls May 23, 2024
@TimGoll
Copy link
Member

TimGoll commented May 23, 2024

I renamed the issue to reflect that

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/enhancement Enhancement or simple change to existing functionality
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants