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

feat(color): add FromStr implementation for Color #180

Merged
merged 7 commits into from
May 20, 2023

Conversation

Mehrbod2002
Copy link
Contributor

fixes: #143

Copy link
Member

@joshka joshka left a comment

Choose a reason for hiding this comment

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

There's a note in the issue about whether we should / shouldn't bring in a new dependency for this (it seems like a fairly small amount of code to avoid doing so). I could go either way, but lean towards not bringing it in. Do we gain anything by putting the implementation off onto colorsys?

src/style.rs Outdated Show resolved Hide resolved
src/style.rs Outdated Show resolved Hide resolved
@Mehrbod2002 Mehrbod2002 requested a review from joshka May 19, 2023 10:10
Copy link
Collaborator

@sophacles sophacles left a comment

Choose a reason for hiding this comment

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

Code looks good to me.

Would like to see documentation - its non-obvious how to make Indexed and Rgb from a string without reading the code.

@orhun
Copy link
Sponsor Member

orhun commented May 19, 2023

I think we should also add unit tests for this.

@Mehrbod2002
Copy link
Contributor Author

I think we should also add unit tests for this.

The tests added

Copy link
Member

@joshka joshka left a comment

Choose a reason for hiding this comment

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

Nice work. This looks good to me.

sophacles
sophacles previously approved these changes May 20, 2023
@sophacles sophacles dismissed their stale review May 20, 2023 15:03

I thought of some edge cases

Copy link
Collaborator

@sophacles sophacles left a comment

Choose a reason for hiding this comment

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

There's some edge cases that we probably want to deal with.

src/style.rs Show resolved Hide resolved
src/style.rs Outdated Show resolved Hide resolved
Copy link
Sponsor Member

@orhun orhun left a comment

Choose a reason for hiding this comment

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

Neat.

@orhun orhun changed the title fix: FromStr in Color Struct feat(color): add FromStr implementation for Color May 20, 2023
@orhun orhun merged commit a425102 into ratatui-org:main May 20, 2023
7 checks passed
@Mehrbod2002 Mehrbod2002 deleted the fromstr_color branch May 20, 2023 18:00
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.

Support conversion from String to Color
4 participants