-
-
Notifications
You must be signed in to change notification settings - Fork 346
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(block): support for having more than one title #232
Conversation
Codecov Report
@@ Coverage Diff @@
## main #232 +/- ##
==========================================
+ Coverage 82.38% 82.50% +0.12%
==========================================
Files 35 36 +1
Lines 7197 7301 +104
==========================================
+ Hits 5929 6024 +95
- Misses 1268 1277 +9
|
Can you explain the need for this a bit more. How would you explain the feature to someone that hasn’t seen btop? I took a look at the screenshot and can’t identify how multiple titles is useful. |
I believe it would allow for a better user experience. For example, similarly to Btop, the titles can be useful for indicating to users keyboard shortcuts they can make use of in a block; one might also want to add additional information in a block's border such as a status bar with word counts, current location, and such. Here are other uses I found:
|
Sorry, I think I wasn't clear. Help me understand how the desired state is desirable. I'm looking for a description of the feature that tells me the effect of this change. How does it render? What are the features that it implements and how do they interact (multiple titles, alignment, on_bottom)? How does it interact with the current features of the block / borders? For |
Oops, sorry for the misunderstanding, I'm new to contributing. For For the rest: NoveltiesI introduced a new title struct in a separate file similar to what has been done with How does it render?If Otherwise, it uses a very similar rendering approach to the old system inside the
Being so similar to the prior system, I doubt there will be any differences between using the new system over the old one. This is why I believe we can deprecate the previous system. |
Got it I wonder if we can change Block::default()
.title(Title::default()
.content("Left")
.alignment(Alignment::Left)
.style(Style::default().fg(Color::Yellow))
.title(Title::default().content("Center").alignment(Alignment::Center))
.title(Title::default().content("Right").alignment(Alignment::Right))
.title(Title::default().content("Left").alignment(Alignment::Left).position(Position::Bottom))
.title(Title::default().content("Center").alignment(Alignment::Center)).position(Position::Bottom)
.title(Title::default().content("Right").alignment(Alignment::Right)).position(Position::Bottom) Thoughts? |
Yes, true. I believe that to be a viable option. I'll work on it sometimes soon. |
I implemented the suggestion you made but had to remove the old system's I'm now uncertain whether or not to bring them back for backward compatibility. As reading them would mean people won't have to make any changes to their current library. We could just mark them deprecated. Please let me know your opinion on this. |
I think leave them in if possible and mark them deprecated. |
I added tests, and brought back Backward compatibility-wise, people shouldn't have to change anything. Thought I haven't tested this. |
The examples are generally good for checking the backwards compatibility as they show what a user will see if they are using the deprecated methods. Assume the next major version for a deprecation (v0.22.0). |
All done I believe. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm generally approving the idea. Grouping the feedback from this review:
- consider whether to make the existing block methods act as defaults for each title. This makes it consistent with other widgets that do this (like
Paragraph::style()
which acts as a default) - make sure the docs cover the edge cases (overlapping), note the defaults, and indicate how the titles will be rendered. (it might be useful to show a couple of example outputs in the docs to make it easy to visualize this)
A lot of my comments come down to picking an API that:
I think that Block::new()
.title("A")
.title("B")
.title_style(Style::default()...)
.title_position(Position::Bottom) is fine |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still looking for some changes on this to make it easy to see how it works.
The main change needed is to render any title style, position or alignment as an override in the render rather than by modifying the stored value.
P.s. Thanks for sticking with it on this review. I think the next change you make will probably be ready to merge. When you push can you please squash the changes into a single commit and update the commit message to use the conventional commit format with a header and a body that describes the feature enough that it works well for the changelog. I didn't mention in the review, but would like to see it some tests that show how overlapping titles are handled (e.g. what happens when there is only 10 characters wide and there are 3 titles (left, center, right aligned) and they are each 4 characters wide...?) |
I tried using Commitizen for the commit, message but it would seem not to have worked. |
44e5dc6
to
eec56ce
Compare
I believe everything to now be ready. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please rebase on main (there's an updated typos.toml that has a fix for the ratatui typo) and fix up the typos (use typos --write-changes
?
Wait, did I mess something up while rebasing? I'm new to this and I'm not sure. |
Yes maybe, but it's not the end of the world. I'd recommend taking the following steps:
|
f824305
to
5bab461
Compare
Thanks a lot, that really helped. And, sorry for the extra work. |
To be able to create TUIs such as that of btop, I believe it to be necessary to be able to have multiple titles for a single block. This pull request adds the ability to have multiple titles per block. Each title can have a different alignment thought multiple titles can have the same alignment.
For backwards compatibility sake, I let the old title system and just added a new one that takes precedence over the old one in cases it has been set.
In the new system, you declare your titles in the following ways:
where the content is just a
Line
like in the old system.Note: I haven't added tests, documentation, or bothered with code style.