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

Add alignment property to Text #6

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

chrisbouchard
Copy link

@chrisbouchard chrisbouchard commented Nov 27, 2022

This merge request introduces an alignment property to the Text component, and a new Alignment enum with alignment options (left, center, right).

Breaking Change

This is a breaking change in that Text::new now takes an additional parameter — since that would be a breaking change no matter what, I added alignment at the beginning of render's parameters to preserve alphabetical order.

Implementation

Under the covers, we directly translate the Alignment to a tui-rs Alignment and set it onto the underlying Paragraph. The default is Alignment::Left, so there should be no change in default behavior.

I also refactored the components::text module a bit, so that it could become a public module (to expose Alignment) without double-exposing the Text component — I followed the pattern from components::stack of creating a pub(super) submodule to hide the component. I couldn't think of a good submodule name, so I used component. I considered using the component name “text,” but components::text::text seemed more confusing than helpful. I'm open to any suggestions, including using “text” if you prefer;

Fixes #5.

I want to introduce a submodule for alignment, but then I'll need to make the
`text` module public, which will expose the public `Text` component definition,
resulting in it appearing in two different modules. To avoid confusion, I've
moved it to a new `text::component` submodule, only visible to super, like we
have with `stack::horizontal` and `stack::vertical`.

I'm not at all sold on the name `component`, but I can't think of anything
better at the moment -- I thought of using the component name, but `text::text`
seemed more confusing than helpful (and Clippy warns about it).
Nothing's using it yet, but the `Text` component will soon.
And pass it through to the underlying `Paragraph`. Since property order doesn't
matter for anyone using `render!`, and any call to `Text::new` is going to break
with an added parameter anyway, I've added the new parameter at the beginning to
preserve alphabetical order.
Now that the `Text` component won't appear twice, we can make the module public
to expose `Alignment`.
@chrisbouchard
Copy link
Author

I could also see putting Alignment somewhere else, e.g., if it might some day also be used to align the title on Section.

@enricozb
Copy link
Owner

enricozb commented Nov 28, 2022

Thank you for the PR.

I'm actually in the process of rewriting intuitive completely because of a fundamental flaw (see #4). This rewrite that is in progress also removes tui-rs as a dependency in favor of a custom drawing solution. Furthermore, this re-rewrite will also pave the way for a focusing system (see #3) and precise re-renders using signals.

Your suggestion to add Alignment to Text, and possibly Section, is a really good one, and I'll consider adding it to the rewrite as well. I'm trying to have it close to feature parity with v0.6 before pushing any code.

Let me think a bit about merging this PR, since so much will change in the upcoming v0.7 anyways.

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.

Add an alignment property to the Text component
2 participants