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.

Sign up for GitHub

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

Jump to bottom

refactor: Create single utility type to encapsulate color argument #107

Merged
merged 4 commits into from
Jul 19, 2021
Merged

refactor: Create single utility type to encapsulate color argument #107

merged 4 commits into from
Jul 19, 2021

Conversation

shuta13
Copy link
Contributor

@shuta13 shuta13 commented Jul 19, 2021 •
edited
Loading

Why

What

Fix: #29

Checklist

  • Added myself to contributors table
  • Ready to be merged

joshuaellis
joshuaellis approved these changes Jul 19, 2021
View reviewed changes
Copy link
Member

@joshuaellis joshuaellis left a comment

Choose a reason for hiding this comment

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

This looks good to me @Methuselah96 it would be good to get your opinion too 👍🏼

@joshuaellis joshuaellis requested a review from Methuselah96 July 19, 2021 07:07
Methuselah96
Methuselah96 approved these changes Jul 19, 2021
View reviewed changes
Copy link
Contributor

@Methuselah96 Methuselah96 left a comment

Choose a reason for hiding this comment

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

Looks good to me. CapsulatedColor probably isn't my preferred term because "capsulated" makes me think there's a value "capsulated" inside an object instead of a union, but I can't think of any obviously better names at the moment.

@joshuaellis
Copy link
Member

joshuaellis commented Jul 19, 2021 •
edited
Loading

UnitedColor? or UColor?

@Methuselah96
Copy link
Contributor

Yeah, either of those work for me. Maybe ColorRepresentation (although that's a bit longer)?

@joshuaellis
Copy link
Member

I prefer ColorRepresentation @shuta13 would you mind making that change please? 🙏🏼

@shuta13
Copy link
Contributor Author

shuta13 commented Jul 19, 2021

@Methuselah96 @joshuaellis

OK, changed. Please check out! : 7aca3fb

Methuselah96
Methuselah96 approved these changes Jul 19, 2021
View reviewed changes
Copy link
Contributor

@Methuselah96 Methuselah96 left a comment

Choose a reason for hiding this comment

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

🎉

@joshuaellis joshuaellis merged commit abe3dd8 into three-types:master Jul 19, 2021
@xawill xawill mentioned this pull request Oct 8, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joshuaellis joshuaellis joshuaellis approved these changes

@Methuselah96 Methuselah96 Methuselah96 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Create single utility type to encapsulate color argument
3 participants
@shuta13 @joshuaellis @Methuselah96