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(widget): support adding padding to Block #20

Merged
merged 9 commits into from
Apr 27, 2023
Merged

Conversation

orhun
Copy link
Member

@orhun orhun commented Feb 12, 2023

Upstream: #551

Description

If we want to render a widget inside a block with a certain distance from its borders, we need to create another Layout element based on the outer Block, add a margin and render the Widget into it. Adding a padding property on the block element skips the creation of this second Layout.

This property works especially when rendering texts, as we can just create a block with padding and use it as the text wrapper:

let block = Block::default().borders(Borders::ALL).padding(4);
let paragraph = Paragraph::new("example paragraph").block(block);
f.render_widget(paragraph, area);

Rendering another widget should be easy too, using the .inner method:

let block = Block::default().borders(Borders::ALL).padding(4);
let inner_block = Block::default().borders(Borders::ALL);
let inner_area = block.inner(area);

f.render_widget(block, area);
f.render_widget(inner_block, inner_area);

Testing guidelines

The block.rs example contains two cases of use: one with an inner text and another with an inner block:
image

I've also changed the paragraph tests to include a padding of value 1 to the rendering tests.

Checklist

@orhun
Copy link
Member Author

orhun commented Feb 12, 2023

alpha-tango-kilo said: This would be awesome! I hope this gets merged 🙌

@mindoodoo mindoodoo added the Type: Enhancement New feature or request label Feb 14, 2023
@appsubscribe
Copy link

Hi, do you know when we can merge this PR? it would be great if this gets merged. Thanks!

@sayanarijit
Copy link
Member

lgtm from me.
Let's resolve the conflict.

@orhun
Copy link
Member Author

orhun commented Apr 20, 2023

I resolved the conflict, please review @mindoodoo @sayanarijit @sophacles @joshka

@orhun orhun changed the title Feature: Block padding feat(widget): support adding padding to Block Apr 20, 2023
@pythops
Copy link
Contributor

pythops commented Apr 20, 2023

Waiting for that one 👀
Thanks @orhun 🙏

Copy link
Contributor

@sophacles sophacles left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good, and is functionality I was looking for not long ago. Curious about what you and the other reviewers think of my suggestion above

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

joshka commented Apr 21, 2023

I'd suggest going left,right,top,bottom on the padding values instead of just horizontal/vertical. With vertical and horizontal being pub fields, that's a breaking change if done later.

Padding will be useful to insert between other things (e.g. ListItems). There at least I'd expect that the top padding to be able to be set differently to the bottom padding (and there are probably use cases for left/right there too). Even the block seems like it would maybe be useful to see different values on each edge.

@mindoodoo mindoodoo removed their request for review April 21, 2023 15:12
@sayanarijit
Copy link
Member

sayanarijit commented Apr 22, 2023

Since we're going with border style attributes, can we use the similar bitwise API as borders?

Nope, sorry, we need the values for each side, which isn't possible with bitwise API.

@joshka
Copy link
Member

joshka commented Apr 22, 2023

LGTM (I think the only thing I can see is missing is a test for the left/right/top/bottom)

@orhun orhun merged commit 782820c into main Apr 27, 2023
@orhun orhun deleted the 551/feature/block-padding branch April 27, 2023 10:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Enhancement New feature or request
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

8 participants