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

Add underline color style for crossterm backend #310

Merged
merged 1 commit into from
Jul 15, 2023
Merged

Add underline color style for crossterm backend #310

merged 1 commit into from
Jul 15, 2023

Conversation

Nogesma
Copy link
Contributor

@Nogesma Nogesma commented Jul 14, 2023

Fixes #308.

@Nogesma Nogesma mentioned this pull request Jul 14, 2023
Closed
@Nogesma Nogesma force-pushed the main branch 3 times, most recently from af916d6 to 2f2e457 Compare July 14, 2023 00:31
@codecov
Copy link

codecov bot commented Jul 14, 2023 •
edited
Loading

Codecov Report

Merging #310 (6450891) into main (2889c7d) will decrease coverage by 0.02%.
The diff coverage is 89.85%.

@@            Coverage Diff             @@
##             main     #310      +/-   ##
==========================================
- Coverage   83.85%   83.84%   -0.02%     
==========================================
  Files          38       38              
  Lines        8045     8082      +37     
==========================================
+ Hits         6746     6776      +30     
- Misses       1299     1306       +7
Impacted Files Coverage Δ
src/backend/crossterm.rs 0.00% <0.00%> (ø)
src/text.rs 71.20% <ø> (ø)
src/buffer.rs 93.44% <100.00%> (+0.13%) ⬆️
src/style.rs 89.38% <100.00%> (+0.46%) ⬆️
src/widgets/gauge.rs 87.44% <100.00%> (+0.23%) ⬆️

@joshka
Copy link
Member

joshka commented Jul 14, 2023

That clippy error seems unrelated - probably due to today's rust 1.71.0 update.

@Nogesma
Copy link
Contributor Author

Nogesma commented Jul 14, 2023 •
edited
Loading

The clippy check seems to fail on a file I haven't touched, and I can't replicate it on my computer.
I think I can actually remove the set_underline_color in buffer.rs as it's not used anywhere, and the coverage fails because of that.

EDIT: I was still on 1.70 on my computer, seems like 1.71 is the culprit.

@joshka
Copy link
Member

joshka commented Jul 14, 2023

Fix for lint is in #311

joshka
joshka requested changes Jul 14, 2023
View reviewed changes
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 generally. It's a small targeted change. I'll wait for @orhun's feedback on this before merging.

src/style.rs Outdated Show resolved Hide resolved
@Nogesma
feat(style): Enable setting the underline color for crossterm (ratatu…
6450891 
…i#308)

This commit adds the underline_color() function to the Style and Cell
structs. This enables setting the underline color of text on the
crossterm backend. This is a no-op for the termion and termwiz backends
as they do not support this feature.
@Nogesma
Copy link
Contributor Author

Nogesma commented Jul 14, 2023

Rebased on top of main and added a link to some documentation.

@joshka
Copy link
Member

joshka commented Jul 14, 2023 •
edited
Loading

The current fail is a code coverage reduction can you please add a couple of tests to make that happy?

@Nogesma
Copy link
Contributor Author

Nogesma commented Jul 14, 2023 •
edited
Loading

I'm not sure I understand why it says it's a reduction in coverage. The coverage went up on all the files I modified.
The only function I added is tested, I'm not sure what I can do more.

@joshka
Copy link
Member

joshka commented Jul 14, 2023

I just too a better look. The missing coverage is on the crossterm backend (which is at 0% coverage), apologies for the wrong direction.

orhun
orhun approved these changes Jul 15, 2023
View reviewed changes
Copy link
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.

LGTM

@orhun orhun added this pull request to the merge queue Jul 15, 2023
Merged via the queue into ratatui:main with commit b347201 Jul 15, 2023
@a-kenji a-kenji mentioned this pull request Jul 16, 2023
Closed
a-kenji pushed a commit to a-kenji/ratatui that referenced this pull request Jul 16, 2023
@Nogesma @a-kenji
feat(style): Enable setting the underline color for crossterm (ratatu…
9b13cef 
…i#308) (ratatui#310)

This commit adds the underline_color() function to the Style and Cell
structs. This enables setting the underline color of text on the
crossterm backend. This is a no-op for the termion and termwiz backends
as they do not support this feature.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@orhun orhun orhun approved these changes

@joshka joshka joshka approved these changes

@mindoodoo mindoodoo Awaiting requested review from mindoodoo

@sayanarijit sayanarijit Awaiting requested review from sayanarijit

@sophacles sophacles Awaiting requested review from sophacles

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

Successfully merging this pull request may close these issues.

Enable setting the underline color
3 participants
@Nogesma @joshka @orhun