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

Tabs: Remove initial space prior to Tabs widget and around tab dividers inside #502

Closed
joshka opened this issue Sep 14, 2023 · 9 comments
Closed
Assignees
Labels
Status: Design Needed An issue that isn't yet designed or specified well enough to implement Type: Enhancement New feature or request

Comments

@joshka
Copy link
Member

joshka commented Sep 14, 2023

Problem

The Tabs widget creates a non configurable space prior to the beginning of the tab, and non removable spaces between the tab text and the divider. E.g. in the following, the space prior to "Inbox" and two of the spaces between "Inbox" and "Sent" are non-removable.

image
Tabs::new(vec![" Inbox ", " Sent ", " Drafts "])
    .style(Style::new().fg(Color::Indexed(244)).bg(Color::Indexed(232)))
    .highlight_style(
        Style::new()
            .bold()
            .fg(Color::Indexed(232))
            .bg(Color::Rgb(64, 96, 192)),
    )
    .select(0)
    .divider("")
    .render(area[0], buf);

Both of these spaces can be got to by alternative means (rendering to an area with a margin, or changing the divider to include spaces either side of the divider)

Solution

  • Remove the initial space
  • Change the default divider to " │ " from "│" and remove the excess spaces

This breaks rendering in two places:

  • Apps that want the extra initial space can fix this by putting a margin around the tabs. E.g.: let area = area.inner(&Margin::(new(1,0)));
  • Apps that use a custom divider can fix this by adding spaces either side of the divider. E.g.: .divider(" • ")

Alternatives

  1. Make the amount of extra space a configuration option with the default set to the current amount
  2. Make the extra space characters configurable with the default option set to a space

I think the main solution is probably reasonable - but so are the alternatives that don't break existing code, so I'd like to get consensus before implementing this.

Additional context

@joshka joshka added Type: Enhancement New feature or request Status: Design Needed An issue that isn't yet designed or specified well enough to implement Effort: Easy Something that should be pretty quick to fix labels Sep 14, 2023
@bhavuk2002
Copy link

i can do that, assign me

@joshka
Copy link
Member Author

joshka commented Sep 16, 2023

Hey there - I'd like us wait a little bit for some feedback from users before we jump to PRs on this:

I think the main solution is probably reasonable - but so are the alternatives that don't break existing code, so I'd like to get consensus before implementing this.

@joshka joshka changed the title Remove spacing for tabs Remove initial space prior to Tabs widget and around tab dividers within the widget Sep 16, 2023
@joshka joshka changed the title Remove initial space prior to Tabs widget and around tab dividers within the widget Remove initial space prior to Tabs widget and around tab dividers inside Sep 16, 2023
@bhavuk2002
Copy link

okay, what can i do i the mean while? as you have changed the title, i have done the same in my PR. can you suggest some good first issue on which i can work on? or for this issue i can do something?

@joshka joshka removed the Effort: Easy Something that should be pretty quick to fix label Sep 16, 2023
@joshka
Copy link
Member Author

joshka commented Sep 16, 2023

I just did a quick pass through the issues and I don't think there's a huge amount of good first issue stuff to do. Most of the current issues require a bit more research / discussion before implementing. The obvious one that might be worth looking at are documentation tasks (which require spending some time with each doc topic and assessing it. See:

I'll see if I can get a few more good first issue friendly issues written up.

@joshka
Copy link
Member Author

joshka commented Sep 17, 2023

The impact of making a change to the way Tabs render would be that any end users using apps built with Ratatui would see the change (especially if they install the app via cargo install without --locked). This means breaking rendering here breaks Ratatui's user's users.

To move this issue towards a solution, we should document in this issue some basic rendering of tabs and note how each approach would change that. E.g.:

Approach 1:

  • Small description of changes to code
  • The following existing code (one or more examples):
    example tab render
    Renders as: Tab1 | Tab2 | Tab3
  • With the change this renders as: Tab1 | Tab2 | Tab3
  • To make the rendering the same as it was previously the app code would have to change to:
    example changed

Approach 2:

  • ...
  • With the change, code that needs to remove the divider / spaces would instead do:
    example of how to use the code
    

etc.

@bhavuk2002 does that sound like something that you could do?

@bhavuk2002
Copy link

Uhm... somewhat im getting it but approaches are unclear i guess have implemented chanes that way already ? Am i right?

@joshka
Copy link
Member Author

joshka commented Sep 17, 2023

Assuming a buffer with:

01234567890123456789012
abc                 xyz

Approach 1 is from the Solution section. It removes the initial space before the tabs and changes the default divider to " | "
So the sample code:

Tabs::new(vec!["123","456","789"]).render(area, buf);

renders:

01234567890123456789012
abc 123 | 456 | 789 xyz

And

Tabs::new(vec!["123","456","789"]).divider("x").render(area, buf);

renders:

01234567890123456789012
abc 123 x 456 x 789 xyz

Changing things for approach 1 changes the renders of those two code to:

01234567890123456789012
abc123 | 456 | 789  xyz
01234567890123456789012
abc123x456x789      xyz
  • "123x456x789"
    To fix these errors, the code examples would have to be updated to...

Approach 2

  • I'm unsure what the methods should look like, but perhaps:
    Tabs::new(vec!["123","456","789"])
      .initial_indent(1)
      .divider_padding(1) // or perhaps (left, right)?
      .render(area, buf);
    I'd expect that the examples above would render the same as they started

Approach 3

  • I'm unsure what the methods should look like, but perhaps:
    Tabs::new(vec!["123","456","789"])
      .initial_prefix(" ")
      .divider_padding(" ")
      .render(area, buf);
    again, the examples above don't change the rendering

I'm not sure if the names of the functions make sense - consider how these would look in docs etc. Consider what would be the natural names for this feature. etc. Think about some of the choices and find some way to justify one vs another.

@bhavuk2002
Copy link

okay im on it

@joshka joshka changed the title Remove initial space prior to Tabs widget and around tab dividers inside Tabs: Remove initial space prior to Tabs widget and around tab dividers inside Sep 28, 2023
rhaskia added a commit to rhaskia/ratatui that referenced this issue Nov 16, 2023
…i#502))

The tab widget now contains the values padding_left/padding_right and the function padding() that sets those values.
rhaskia added a commit to rhaskia/ratatui that referenced this issue Nov 21, 2023
…i#502))

The widget now contains padding_left and and padding_right properties.

Those values can be set with functions padding_left, padding_right, and padding all of which take

Into<Line>.
rhaskia added a commit to rhaskia/ratatui that referenced this issue Nov 21, 2023
…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 pushed a commit that referenced this issue Dec 4, 2023
The Tab widget now contains padding_left and and padding_right
properties. Those values can be set with functions `padding_left()`,
`padding_right()`, and `padding()` whic all accept `Into<Line>`.

Fixes issue #502
@Valentin271
Copy link
Member

This seems to be fixed in #629, closing. Feel free to reopen if I'm wrong.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Design Needed An issue that isn't yet designed or specified well enough to implement Type: Enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants