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(widget): add patch_style to Spans #148

Merged
merged 12 commits into from
May 2, 2023
Merged

Conversation

lthoerner
Copy link
Contributor

@lthoerner lthoerner commented Apr 23, 2023

Added a method to Spans that patches the style of each Span in the Spans similarly to Text.patch_style().

@lthoerner
Copy link
Contributor Author

I'm not sure how all the checks failed because it's literally a drop-in method that does not remove or change anything public

Copy link
Contributor

@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.

I think patch_style could be helpful to widget authors. I think I'd like to see a similar Span.patch_style() also.

src/text.rs Show resolved Hide resolved
@lthoerner
Copy link
Contributor Author

I think patch_style could be helpful to widget authors. I think I'd like to see a similar Span.patch_style() also.

Sorry, just a typo. The method is called patch_style().

@sophacles
Copy link
Contributor

Sorry, just a typo. The method is called patch_style().

I know - the current PR only changes struct Spans, It also like to see patch_style() for the struct Span. (Spans is a collectio of Span)

@sophacles
Copy link
Contributor

Reply to myself:

Span has a public .style, so one could do .style.patch so maybe it's not necessary.

lthoerner and others added 2 commits April 23, 2023 14:11
@lthoerner
Copy link
Contributor Author

Do you guys want Text.reset_style() and Span.reset_style() as well?

@lthoerner lthoerner changed the title Spans.parse_style() Spans.patch_style() Apr 23, 2023
@lthoerner
Copy link
Contributor Author

I tried to make the commit messages follow convention but it duplicated the commits :/

@orhun orhun changed the title Spans.patch_style() feat: add patch_style to Spans May 2, 2023
@orhun orhun changed the title feat: add patch_style to Spans feat(widget): add patch_style to Spans May 2, 2023
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.

Some minor changes, can you squash this into a single commit with a conventional commit like feat(widget): add style methods to Span and Spans (or something similar). Looks like an easy merge once that's done.

src/text.rs Outdated Show resolved Hide resolved
src/text.rs Outdated Show resolved Hide resolved
src/text.rs Outdated Show resolved Hide resolved
@lthoerner
Copy link
Contributor Author

@joshka The requested changes have been made 👍

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.

LGTM. Thanks for your contribution.

@joshka joshka merged commit 33b3f4e into ratatui:main May 2, 2023
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.

4 participants