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(tabs): Tab widget now supports custom padding (Fixes issue #502)) #629

Merged
merged 3 commits into from
Dec 4, 2023

Conversation

rhaskia
Copy link
Contributor

@rhaskia rhaskia commented Nov 16, 2023

The tab widget now contains the values padding_left/padding_right and the function padding() that sets those values.
Should fix issue #502.

Copy link

codecov bot commented Nov 16, 2023

Codecov Report

Attention: 20 lines in your changes are missing coverage. Please review.

Comparison is base (1229b96) 90.7% compared to head (d4d3147) 90.6%.

Files Patch % Lines
src/widgets/tabs.rs 68.7% 20 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##            main    #629     +/-   ##
=======================================
- Coverage   90.7%   90.6%   -0.2%     
=======================================
  Files         42      42             
  Lines      12170   12232     +62     
=======================================
+ Hits       11042   11084     +42     
- Misses      1128    1148     +20     

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

@joshka
Copy link
Member

joshka commented Nov 16, 2023

Cool. I didn't realize that the spacing before the tabs was coming from padding left / right. My mental model without having looked at the code was that the spaces "belonged to the divider. This solution is better than the ones on the Issue.

A question though, what if we made the padding accept &str instead of just a number?

A follow up to this might be to make padding accept Line to make the divider / padding able to be styled easily.
Imagine:

🭊🭁Tab 1🭌🬿 🭊🭁Tab 2🭌🬿

But with the background of the label part the same color as the tab padding parts.

(copied from the discord feedback so that it's recorded for posterity)

src/widgets/tabs.rs Outdated Show resolved Hide resolved
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.

approved by mistake

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.

We can include the padding part of the tabs in the highlight by adding it to the tab titles instead of the padding. So I wonder whether it would be the worst thing to omit the highlight part of this?

Can you please add some docs about padding to the main rustdoc comment for the type? The doc needs info on the defaults for the padding and divider.

I'm going to make a breaking change for highlight_style in another PR as it seems natural to make that actually do something reasonable in the default configuration.

src/widgets/tabs.rs Outdated Show resolved Hide resolved
src/widgets/tabs.rs Outdated Show resolved Hide resolved
src/widgets/tabs.rs Outdated Show resolved Hide resolved
src/widgets/tabs.rs Outdated Show resolved Hide resolved
@joshka
Copy link
Member

joshka commented Nov 20, 2023

Hey, would you mind setting up commit signing?
https://github.com/ratatui-org/ratatui/blob/main/CONTRIBUTING.md#sign-your-commits

@rhaskia
Copy link
Contributor Author

rhaskia commented Nov 20, 2023

I did, I think something went wrong as it says the email does not match the signature.

@joshka
Copy link
Member

joshka commented Nov 20, 2023

Github mangles your email address to [email protected] is that setup as being valid for signing?

@joshka
Copy link
Member

joshka commented Nov 21, 2023

Thanks for fixing up the commit signing - Github still complains that it can't merge them as it needs all the commits to be signed. Can you please squash the commits into a single signed commit and force push the single commit as an update to your branch?

…i#502))\nThe widget now contains padding_left and and padding_right properties.\nThose values can be set with functions padding_left, padding_right, and padding all of which takeInto<Line>.
@joshka joshka merged commit 28ac55b into ratatui:main Dec 4, 2023
31 of 33 checks passed
@joshka
Copy link
Member

joshka commented Dec 4, 2023

Just merged this - thanks again for the PR :)

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.

2 participants