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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(barchart): Add direction attribute. (horizontal bars support) #325

Merged
merged 2 commits into from
Aug 24, 2023

Conversation

karthago1
Copy link
Contributor

@karthago1 karthago1 commented Jul 16, 2023

Fixes #321

this patch introduces new function direction to allow the barchart to render the bars horizontally.

Note: when direction is set to Direction::Horizontal the function Bar::label() is useless, because the bar label is nowhere printed.

any suggestion how to make the horizontal bar look better?

The bar's value_text is rendered using 2 styles (value_style and the bar_style). Reason: see the following image 馃槃

top

@codecov
Copy link

codecov bot commented Jul 16, 2023

Codecov Report

Merging #325 (4de7ecb) into main (a937500) will increase coverage by 0.33%.
The diff coverage is 99.34%.

@@            Coverage Diff             @@
##             main     #325      +/-   ##
==========================================
+ Coverage   87.08%   87.41%   +0.33%     
==========================================
  Files          40       40              
  Lines        9822    10096     +274     
==========================================
+ Hits         8553     8825     +272     
- Misses       1269     1271       +2     
Files Changed Coverage 螖
src/widgets/barchart/bar.rs 98.86% <97.14%> (-1.14%) 猬囷笍
src/widgets/barchart/mod.rs 92.70% <99.61%> (+2.72%) 猬嗭笍
src/widgets/barchart/bar_group.rs 100.00% <100.00%> (酶)

@karthago1 karthago1 force-pushed the hich/barchart branch 2 times, most recently from 71e6fc7 to 0f6e5d5 Compare July 16, 2023 20:11
@karthago1 karthago1 marked this pull request as draft July 18, 2023 15:05
@karthago1 karthago1 marked this pull request as ready for review July 18, 2023 15:15
@kdheepak
Copy link
Collaborator

Thanks for the PR! Horizontal bar charts look great.

It looks like there's a merge conflict in one of the tests. Would you be able to address that?

@ChiaoTeo
Copy link

ChiaoTeo commented Aug 8, 2023

nice pr, hope merge

@karthago1
Copy link
Contributor Author

@kdheepak done

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 definitely is something that will be useful. This needs some docs about how it works and some more detailed testing. There's a couple of places where I'm not sure what the behavior should be, so I'm not sure if it's a bug or an non-intuitive feature that you've considered.

examples/barchart.rs Outdated Show resolved Hide resolved
src/widgets/barchart/bar.rs Show resolved Hide resolved
src/widgets/barchart/bar.rs Outdated Show resolved Hide resolved
src/widgets/barchart/bar.rs Outdated Show resolved Hide resolved
src/widgets/barchart/bar_group.rs Outdated Show resolved Hide resolved
src/widgets/barchart/bar_group.rs Outdated Show resolved Hide resolved
src/widgets/barchart/mod.rs Outdated Show resolved Hide resolved
src/widgets/barchart/mod.rs Outdated Show resolved Hide resolved
src/widgets/barchart/mod.rs Outdated Show resolved Hide resolved
src/widgets/barchart/mod.rs Outdated Show resolved Hide resolved
@karthago1 karthago1 force-pushed the hich/barchart branch 3 times, most recently from 0eeb54c to 5967c49 Compare August 12, 2023 21:29
@karthago1 karthago1 changed the title feat(barchart): Add horizontal_bars attribute feat(barchart): Add direction attribute. (horizontal bars support) Aug 13, 2023
@karthago1 karthago1 force-pushed the hich/barchart branch 2 times, most recently from 0a775fb to 532be7b Compare August 16, 2023 19:38
@karthago1 karthago1 requested a review from joshka August 16, 2023 20:03
@karthago1 karthago1 force-pushed the hich/barchart branch 2 times, most recently from 181b82f to b35af7f Compare August 18, 2023 16:15
@joshka joshka added this to the v0.23.0 milestone Aug 21, 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.

A couple of small typos, but generally looks good to me. If you can get these fixed up in the next few days this should make the 0.23 release.

I'd still query whether bottom to top rendering is the correct approach, but happy to take that as a future PR if needed.

src/widgets/barchart/bar.rs Outdated Show resolved Hide resolved
src/widgets/barchart/mod.rs Outdated Show resolved Hide resolved
@karthago1
Copy link
Contributor Author

karthago1 commented Aug 24, 2023

I'd still query whether bottom to top rendering is the correct approach, but happy to take that as a future PR if needed.

@joshka I don't care much. I slightly like printing from the bottom. here I made a comparison. It will be nice if you choose, which one looks better? (it will not require much time changing it)

top: (ignore the misplaced group labels)
top

(here we should maybe move the legend to the bottom )

botttom:
bottom

@karthago1
Copy link
Contributor Author

karthago1 commented Aug 24, 2023

or maybe let's do in future PR, as you mentioned. maybe we make it configurable. Variable name would be layout_direction or something similar

@joshka
Copy link
Member

joshka commented Aug 24, 2023

or maybe let's do in future PR, as you mentioned. maybe we make it configurable. Variable name would be layout_direction or something similar

Yep that's a good way to handle it

@joshka
Copy link
Member

joshka commented Aug 24, 2023

Let's go top down for the first release and add another property in sometime soon in a .1 release?

@karthago1
Copy link
Contributor Author

Let's go top down for the first release and add another property in sometime soon in a .1 release?

ok fine.

Enable rendring the bars horizontally. In some cases this allow us to
make more efficient use of the available space.

Signed-off-by: Ben Fekih, Hichem <hichem.f@live.de>
This is a breaking change, since the alignment by default is set to
Left and the group labels are always rendered in the center.

Signed-off-by: Ben Fekih, Hichem <hichem.f@live.de>
@karthago1
Copy link
Contributor Author

@joshka done. Now it looks like the following:

top

@joshka
Copy link
Member

joshka commented Aug 24, 2023

LGTM. Thanks again for the PR and for your patience sticking with it to get this reviewed and merged.

@joshka
Copy link
Member

joshka commented Aug 24, 2023

@ratatui-org/core can someone please take a second review on this?

@joshka joshka added this pull request to the merge queue Aug 24, 2023
@joshka
Copy link
Member

joshka commented Aug 24, 2023

Merging! Thanks @karthago1 and @kdheepak for the review

Merged via the queue into ratatui-org:main with commit 0dca6a6 Aug 24, 2023
30 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.

BarChart: horizontal bars
4 participants