Topic Banner

[andymandias]

Initial implementation for #236. Currently it builds on #221, but it can be disentangled if that complicates the process.

[andymandias]

Currently the topic bar searches the channel history (new to old) to find the topic, then puts it in a scrollable at the top of the channel pane. If no topic is found, then no scrollable is created. Included in the topic bar is the user that set it and when it was set, with the user's nick being able to be clicked on if they are in the channel. I made it background-color text on an info color scrollable to keep it distinct from the other views, but picked the darkest shade of the info color to try to reduce how distracting it can be. A maximum height can be set in the config to limit the height of the topic banner, with the default aiming to be two lines tall.

[casperstorm]

It's hard to review it since it builds on top of another PR. I would, if not too complicated, like to have it separate so we can work on them separately. Idea is great though, so I would like to see if we can land this feature.

[andymandias]

Absolutely, sorry about that. Wasn't sure how GitHub would handle the PR, but I can see now it's a bit of a mess. Will disentangle then force push.

[andymandias]

Disentangled :)

[andymandias]

Rebased and modified to take advantage of the serde code @tarkah provided in #221.

[casperstorm]

Also, i played a bit with the styling and think we can tone it a bit down from what you have currently.

[casperstorm]

@tarkah mind taking a look at this as well?

[tarkah]

Let me look tomorrow, sorry for my delay on this

[casperstorm]

I am trying a few different designs tests, will report back tomorrow with some suggestions. Good job on this @andymandias.

[casperstorm]

I've played around with styling a bunch, and I think, for now, what I like the most is actually to keep it as simple as possible, just like the nick_list which uses: theme::Scrollable::Hidden as well as theme::Text::Transparent to blend it more with the pane header. That way we can also remove theme::Scrollable::Banner and theme::Text::Banner. When playing around with this, I found that changing the padding slightly at src\buffer\channel.rs:160 worked the best:

    container(body)
        .width(Length::Fill)
        .height(Length::Fill)
-       .padding(8)
+       .padding([4, 8, 8, 8])
        .into()

Regarding layout i think we should go for a layout like this:

| topic   | users |
|---------|       |
| message |       |

Because it can look a little strange if topic isn't long enough and user list starts below:

Third, and last thing, I think we should add a pane button. Notice the button on the top image. Ive tried a bunch and kinda like the following:

pub fn topic<'a>() -> Text<'a> {
    to_text('\u{f5af}')
}

I think, with a button like this we can simplify the config a little as well and make it similar to Users:

buffer:
    users:
      visible: true

Perhaps we could simply do:

buffer:
    topic:
      visible: false
      max_lines: 2

Functionality should just be 1 to 1 with showing and hiding nick_list.
Let me know what you think about these suggetions, @andymandias.

[andymandias]

Thank you for the notes @casperstorm.

I think these are all good changes, and mesh into the Halloy style better.

Re: padding. By removing the background color from the topic banner we can shrink the padding on it without any aesthetic changes. Over here there's been a downside to the topic banner's current padding where it would show a sliver of the third line when only two lines were otherwise showing. For example with the currently-committed padding I see:

So, unless there's an issue I'm going to remove the vertical padding from the topic banner. I believe that will be equivalent to the padding change you've suggested in channel.rs, so I'm going to hold off on making that change. I have no objection to changing the padding further, I just don't want to overshoot what you're suggesting.

I am slightly concerned that there's not a lot differentiating the topic from the rest of the channel text. Would using something like a light horizontal rule fit within the Halloy style?

A seasoned user probably wouldn't need it, but a newbie might be able to use the extra affordance.

I like the pane button idea quite a bit. I'm assuming toggling the topic button should show/hide the inline topic messages accordingly. Is that right? Or would it only toggle the visibility of the banner?

I think matching configuration options is ideal, but I am concerned it could be confusing to users if they set visible: false and they see inline topic messages. Perhaps the options could be Inline, Banner, or false?

[casperstorm]

1. A divider between topic and content is all good to me. It looks good as you have it.
2. Re the sliver of the third line. Could a solution be adding a border and or background in the same color as the pane so it looks transparent but is colored? Just an idea, haven't tested if it actually works.
3. Re button. My initial thought was that config setting only affected banner, and button was a toggle for it. Similar to user list where you can set visible: false, but still quickly toggle to briefly check. What we could do is consider adding a topic variant to server_messages which controls inline, and then rename topic under buffer to topic_banner with visible: boolean, max_lines: u16? That could solve all cases perhaps.
4. Re padding. I think we should aim to get the spacing between pane header, topic divider and from topic divider to content equal. See below image. Its a super small nit.
   

[andymandias]

Thank you for the clarifications! I'll make the changes, going with both banner and server messages configuration options as you describe. No sweat if we wind up changing the configuration options again :)

[andymandias]

Some chores left to be done (e.g. example config update, code clean-up pass, etc), but I believe this has the new configuration and functionality we're looking for. I struggled a little with the under-topic padding since its apparent height can be influenced by scroll state of the messages in the channel; if I didn't get it right please feel free to fix (or let me know how far off I am).

[casperstorm]

I updated padding slightly, example config file, CHANGELOG and then rebased on main. We should update wiki once we merge.

I don't have anything else - this looks awesome, and what awesome feature. Do you have anything you want to clean up before we merge @andymandias?

[andymandias]

Did a quick review and didn't see anything I made too messy. Looks good to merge to me.

Thanks for taking care of the rest of the chores @casperstorm, and for all the guidance from you and @tarkah.