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(table): Add a Table::segment_size method #660

Merged
merged 1 commit into from
Dec 4, 2023

Conversation

asomers
Copy link
Contributor

@asomers asomers commented Dec 3, 2023

It works just like Layout::segment_size, but for Tables. Previously, it was impossible to distribute extra space anywhere in a Table.

Fixes #370

Copy link

codecov bot commented Dec 3, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

❗ No coverage uploaded for pull request base (main@6b2efd0). Click here to learn what that means.

Additional details and impacted files
@@          Coverage Diff           @@
##             main    #660   +/-   ##
======================================
  Coverage        ?   90.9%           
======================================
  Files           ?      42           
  Lines           ?   12502           
  Branches        ?       0           
======================================
  Hits            ?   11373           
  Misses          ?    1129           
  Partials        ?       0           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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 - 1 small nit.

Can you please update the commit message (or provide one in a comment) that describes the behavior directly rather than in reference to Layout. This will end up in the Changelog, so think about how you'd announce the functionality rather than just describing the change and perhaps add 3 small code examples like

let area = Rect::new(0, 0, 62, 1);
let widths = [Min(10), Min(10, Min(10)];
// results in column widths 10, 10, 10
Table::new(rows).widths(widths).segment_size(SegmentSize::None);
// ...

src/widgets/table.rs Show resolved Hide resolved
src/widgets/table.rs Show resolved Hide resolved
@asomers
Copy link
Contributor Author

asomers commented Dec 4, 2023

BTW, for an example of this new feature in action, see asomers/ztop#33 .

@asomers
Copy link
Contributor Author

asomers commented Dec 4, 2023

Does your CI process merge PRs with the main branch before running tests? Because the test that's failing is one that isn't in my branch. Neat, if true.

@joshka
Copy link
Member

joshka commented Dec 4, 2023

Does your CI process merge PRs with the main branch before running tests? Because the test that's failing is one that isn't in my branch. Neat, if true.

Yep :)
If it was the tabs - I just fixed that issue, so if you rebase it should be fine

@asomers
Copy link
Contributor Author

asomers commented Dec 4, 2023

Rebased. And I squashed my commits to satisfy the commit style checker.

@orhun orhun requested a review from joshka December 4, 2023 19:09
It controls how to distribute extra space to an underconstrained table.
The default, legacy behavior is to leave the extra space unused.  The
new options are LastTakesRemainder which gets all space to the rightmost
column that can used it, and EvenDistribution which divides it amongst
all columns.

Fixes ratatui-org#370
@joshka joshka merged commit e49385b into ratatui-org:main Dec 4, 2023
33 checks passed
@asomers asomers deleted the table.segment_size branch December 18, 2023 15:50
joshka added a commit that referenced this pull request Jan 5, 2024
In #660 we introduced the
segment_size field to the Table struct. However, we forgot to update
the default() implementation to match the new() implementation. This
meant that the default() implementation picked up SegmentSize::default()
instead of SegmentSize::None.

Additionally the introduction of Table::default() in an earlier PR,
#339, was also missing the
default for the column_spacing field (1).

This commit fixes the default() implementation to match the new()
implementation of these two fields by implementing the Default trait
manually.

BREAKING CHANGE: The default() implementation of Table now sets the
column_spacing field to 1 and the segment_size field to
SegmentSize::None. This will affect the rendering of a small amount of
apps.
joshka added a commit that referenced this pull request Jan 5, 2024
In #660 we introduced the
segment_size field to the Table struct. However, we forgot to update
the default() implementation to match the new() implementation. This
meant that the default() implementation picked up SegmentSize::default()
instead of SegmentSize::None.

Additionally the introduction of Table::default() in an earlier PR,
#339, was also missing the
default for the column_spacing field (1).

This commit fixes the default() implementation to match the new()
implementation of these two fields by implementing the Default trait
manually.

BREAKING CHANGE: The default() implementation of Table now sets the
column_spacing field to 1 and the segment_size field to
SegmentSize::None. This will affect the rendering of a small amount of
apps.
Valentin271 pushed a commit that referenced this pull request Jan 10, 2024
In #660 we introduced the
segment_size field to the Table struct. However, we forgot to update
the default() implementation to match the new() implementation. This
meant that the default() implementation picked up SegmentSize::default()
instead of SegmentSize::None.

Additionally the introduction of Table::default() in an earlier PR,
#339, was also missing the
default for the column_spacing field (1).

This commit fixes the default() implementation to match the new()
implementation of these two fields by implementing the Default trait
manually.

BREAKING CHANGE: The default() implementation of Table now sets the
column_spacing field to 1 and the segment_size field to
SegmentSize::None. This will affect the rendering of a small amount of
apps.
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.

Table: Add option to make expand_to_fill configurable for Table::widths
3 participants