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: stylization shorthands #283

Merged
merged 1 commit into from
Jul 1, 2023
Merged

Conversation

samyosm
Copy link
Contributor

@samyosm samyosm commented Jun 29, 2023

fix: #241

This PR adds the ability to use style shorthands to str, Span, and Paragraph.

Examples

assert_eq!(
    "hello".red().on_blue().bold(),
    styled("hello", Style::default().fg(Color::Red).bg(Color::Blue).add_modifier(Modifier::BOLD))
)

@samyosm samyosm changed the title reduce boilerplate on every style() call feat: stylization shorthands Jun 29, 2023
@samyosm samyosm force-pushed the less-style-boilerplate branch from 4b33e14 to f8a6b10 Compare June 29, 2023 23:01
@samyosm samyosm marked this pull request as ready for review June 29, 2023 23:07
@codecov
Copy link

codecov bot commented Jun 29, 2023

Codecov Report

Merging #283 (870f88a) into main (83d3ec7) will increase coverage by 0.24%.
The diff coverage is 94.82%.

❗ Current head 870f88a differs from pull request most recent head dda06fd. Consider uploading reports for the commit dda06fd to get more accurate results

@@            Coverage Diff             @@
##             main     #283      +/-   ##
==========================================
+ Coverage   82.71%   82.95%   +0.24%     
==========================================
  Files          36       37       +1     
  Lines        7360     7476     +116     
==========================================
+ Hits         6088     6202     +114     
- Misses       1272     1274       +2     
Impacted Files Coverage Δ
src/style.rs 85.65% <ø> (ø)
src/widgets/paragraph.rs 98.70% <0.00%> (-1.11%) ⬇️
src/style/stylized.rs 100.00% <100.00%> (ø)
src/text.rs 71.20% <100.00%> (+1.45%) ⬆️
src/widgets/block.rs 85.12% <100.00%> (+0.96%) ⬆️

@samyosm samyosm force-pushed the less-style-boilerplate branch from f8a6b10 to b011d1d Compare June 29, 2023 23:33
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.

I'd like to see how this works for styled things outside this module. What imports are needed to make it work, and what do the calls for real-ish ui code look like? E.g. #241 (comment))

Can you please add some integration test for this in tests/stylize.rs?

I suspect that it might make sense to move the impl Styled code to e.g. paragraph.rs etc.

src/text/stylized.rs Outdated Show resolved Hide resolved
src/text/stylized.rs Outdated Show resolved Hide resolved
src/text/stylized.rs Outdated Show resolved Hide resolved
src/widgets/paragraph.rs Outdated Show resolved Hide resolved
src/text/stylized.rs Outdated Show resolved Hide resolved
@samyosm samyosm force-pushed the less-style-boilerplate branch 2 times, most recently from 095b8d7 to 4452bd4 Compare June 30, 2023 03:22
@samyosm
Copy link
Contributor Author

samyosm commented Jun 30, 2023

Can you please add some integration test for this in tests/stylize.rs?

All the necessary tests are already in src/text/stylized.rs. Would it not be better to modify the already existing tests in the tests folder to use this new feature instead? Not just to put it to the test, but also to advocate its usage?

@joshka
Copy link
Member

joshka commented Jun 30, 2023

All the necessary tests are already in src/text/stylized.rs. Would it not be better to modify the already existing tests in the tests folder to use this new feature instead? Not just to put it to the test, but also to advocate its usage?

My thinking behind this was that usually I'd organize tests which deal with just the things defined in a module within that module (e.g. here it's just the traits, which don't actually need tests of themselves - which probably means that they could just be added in style.rs perhaps?) Whereas the stylized methods are only really part of the {Text, Paragraph, str ... } when the traits are also imported. So we could add them to text.rs, paragraph.rs etc. but I think it probably makes sense to have them in a single place to make it easy to see how they work.

Does that make sense or am I overthinking it?

Edit: Put a simpler way, unit tests show that the individual types in a specific module work. Integration tests show that modules work together from the perspective of someone using them.

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.

This is looking pretty good.

I'd like some of the other devs to sanity check this (ping @sayanarijit @orhun), but I think it will be good once the missing tests and docs are added.

When adding the tests, have a think about the user of this and what the imports look like. I think it might make more sense for a use to find and import this feature as style::Stylize" instead of text::Stylize`. If so, let's move the entire stylized file under the style mod and re-export the trait(s).

src/text/stylized.rs Outdated Show resolved Hide resolved
src/text/stylized.rs Outdated Show resolved Hide resolved
src/text/stylized.rs Outdated Show resolved Hide resolved
src/text/stylized.rs Outdated Show resolved Hide resolved
src/text/stylized.rs Outdated Show resolved Hide resolved
src/text.rs Show resolved Hide resolved
src/text/stylized.rs Outdated Show resolved Hide resolved
src/widgets/paragraph.rs Show resolved Hide resolved
@SLASHLogin
Copy link
Contributor

If this is the preferred way of stylizing text, I think it would be nice to convert examples to use those changes. This would also show how the changes would look like from the user's perspective.

@samyosm samyosm force-pushed the less-style-boilerplate branch 2 times, most recently from 93e77d9 to 368c251 Compare June 30, 2023 22:51
@joshka
Copy link
Member

joshka commented Jul 1, 2023

Looking at the examples convinces me that this is really useful (and that stylize does indeed make sense being in the style module).

What documentation is necessary to help users find and understand this feature in the generated docs? (Happy to make that another PR if that would work better?)

@joshka joshka self-requested a review July 1, 2023 00:27
@samyosm
Copy link
Contributor Author

samyosm commented Jul 1, 2023

Though styling is introduced here, I think I will add a short explanation for using style and the shorthands here instead, at the module level right such that a user sees it right after clicking the Style module link in the homepage.

By the way, should I advocate the use of style variables or not?

@samyosm samyosm force-pushed the less-style-boilerplate branch from 368c251 to 870f88a Compare July 1, 2023 01:07
@joshka joshka self-requested a review July 1, 2023 09:10
@joshka joshka enabled auto-merge July 1, 2023 09:11
@joshka joshka force-pushed the less-style-boilerplate branch from 870f88a to dda06fd Compare July 1, 2023 09:14
@joshka joshka added this pull request to the merge queue Jul 1, 2023
Merged via the queue into ratatui:main with commit 2f4413b Jul 1, 2023
@joshka
Copy link
Member

joshka commented Jul 1, 2023

By the way, should I advocate the use of style variables or not?

It's neither here nor there whether people use inline, variables or the easier methods. Each has their pros and cons.

@joshka
Copy link
Member

joshka commented Jul 1, 2023

Thanks for your effort in this.

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.

Reduce boilerplate on every style() call
3 participants