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

fix(chart): fix chart row style being incorrect when using Axis::title #462

Merged
merged 1 commit into from
Sep 5, 2023

Conversation

aatukaj
Copy link
Contributor

@aatukaj aatukaj commented Sep 2, 2023

Fixes: #379

old behavior
image

new fixed
image

@codecov
Copy link

codecov bot commented Sep 2, 2023

Codecov Report

Merging #462 (b22a249) into main (e098731) will increase coverage by 0.06%.
Report is 4 commits behind head on main.
The diff coverage is 100.00%.

❗ Current head b22a249 differs from pull request most recent head 2341fb6. Consider uploading reports for the commit 2341fb6 to get more accurate results

@@            Coverage Diff             @@
##             main     #462      +/-   ##
==========================================
+ Coverage   90.00%   90.06%   +0.06%     
==========================================
  Files          40       40              
  Lines       11156    11166      +10     
==========================================
+ Hits        10041    10057      +16     
+ Misses       1115     1109       -6     
Files Changed Coverage Δ
src/widgets/chart.rs 98.52% <100.00%> (+0.67%) ⬆️

... and 1 file with indirect coverage changes

@aatukaj
Copy link
Contributor Author

aatukaj commented Sep 2, 2023

I'm not really sure on how to go about fixing that commit lint :(

@aatukaj aatukaj requested a review from joshka September 2, 2023 15:23
@joshka
Copy link
Member

joshka commented Sep 2, 2023

I'm not really sure on how to go about fixing that commit lint :(

Mainly this would be fixed by changing the word "added" to "add"
In this case, I'll squash the two commits during the merge, and add some context to the commit message body, as the additional test really is part of the main change and doesn't really need a separate commit.

@joshka
Copy link
Member

joshka commented Sep 2, 2023

Actually - change of plans, double checking this locally, the conventional commit lint prevented the formatting check to run, and there's an extraneous newline that the formatter corrects. Can you please squash the two commits using git rebase and then run cargo +nightly fmt --all and force push the result to your github branch (git push origin main --force)?

Do you think it would be worth adding another unit test that also checks that the data style is not overwritten by the title / axis style?

Here's a suggestion for an improved commit message. I'm not sure if I've correctly noted that it's the dataset style not the title style here.

fix(chart): use graph style for top line

A bug in the rendering caused the top line of the chart to be rendered
using the style of the Y Axis, instead of the dataset style. This is
fixed by only setting the style for the width of the text, and not the
entire row.

Fixes: https://github.com/ratatui-org/ratatui/issues/379

@joshka joshka self-requested a review September 2, 2023 22:12
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.

Actually - change of plans, double checking this locally, the conventional commit lint prevented the formatting check to run, and there's an extraneous newline that the formatter corrects. Can you please squash the two commits using git rebase and then run cargo +nightly fmt --all and force push the result to your github branch (git push origin main --force)?

Do you think it would be worth adding another unit test that also checks that the data style is not overwritten by the title / axis style?

Here's a suggestion for an improved commit message. I'm not sure if I've correctly noted that it's the dataset style not the title style here.

fix(chart): use graph style for top line

A bug in the rendering caused the top line of the chart to be rendered
using the style of the Y Axis, instead of the dataset style. This is
fixed by only setting the style for the width of the text, and not the
entire row.

Fixes: https://github.com/ratatui-org/ratatui/issues/379

@aatukaj
Copy link
Contributor Author

aatukaj commented Sep 3, 2023

Added the test. Hopefully I did the rebase correctly. Thanks for the commit suggestion.

@joshka
Copy link
Member

joshka commented Sep 4, 2023

Thanks for adding the test. LGTM. One last thing (unless the other maintainers have comments on the PR):

We use commit signature verification, which will block commits from being merged unless they are signed. To set up your machine to sign commits, see managing commit signature verification in GitHub docs.

Also not a big deal, the commit message doesn't really need the following lines as writing tests can be generally considered as part of a feature:

test(chart): add test for too big titles
test(chart): add test for correct top line styling

A bug in the rendering caused the top line of the chart to be rendered
using the style of the chart, instead of the dataset style. This is
fixed by only setting the style for the width of the text, and not the
entire row.

Fixes: ratatui#379
@aatukaj
Copy link
Contributor Author

aatukaj commented Sep 4, 2023

Everything should be good now.
Thanks for all the help!

@joshka joshka merged commit c8ab2d5 into ratatui:main Sep 5, 2023
28 of 29 checks passed
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.

Top line chart dot was styled with Y Axis color
3 participants