Skip to content

Conversation

@Osaedasia
Copy link

Closes #1812

+ header methods

- Removed `new_with_header` and `new_with_spaced_header` constructors.
- Introduced single `new(label)` constructor combined with chainable
  methods:
  - `header()` for a fully custom header
  - `spaced_header()` for left/right aligned header
  - `with_header_style()` to override default header styling
- Rationale: simplifies API, avoids duplicating label/header
  information, and aligns with GPUI/shadcn-ui builder patterns.
- Maintains backward compatibility: simple label usage still works with
  `new()`.
@huacnlee
Copy link
Member

The Sidebar is allowing to add any elements (impl Collapsible) as child, would you try build your own SidebarGroup?

I have read the API that you added. It's looks like not common for most case, more like for some little use case. I don't think those APIs may not necessary.

/// Convenience method for aligning content at the left and right of the header.
///
/// **Warning:** This replaces the label from [`SidebarGroup::new`].
pub fn spaced_header(self, left: impl IntoElement, right: impl IntoElement) -> Self {
Copy link
Member

Choose a reason for hiding this comment

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

This API is not reasonable to as a basic API.

Copy link
Author

Choose a reason for hiding this comment

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

I removed it from SidebarGroup

///
/// The closure receives the default [`Div`] used for the header and should return a
/// customized [`Div`]. This allows changing padding, color, rounding, height, etc.
pub fn with_header_style<F>(mut self, f: F) -> Self
Copy link
Member

Choose a reason for hiding this comment

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

The fn header can handle the style refine totally.

Copy link
Author

Choose a reason for hiding this comment

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

I removed it from SidebarGroup

Copy link
Member

@huacnlee huacnlee left a comment

Choose a reason for hiding this comment

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

To change the label as option, to allows SidebarGroup to more flexible, is reasonable. If the API is good, it can be merged.

But, your current design not good enough, please do more consider the API, and read the exists components in this project and reference some other UI library (like Shadcn), then make change.

@Osaedasia
Copy link
Author

I followed your first suggestion and decided to go with a new group type for the sidebar instead of extending the existing API.

I’m now building a dedicated SidebarGroup that implements Collapsible and can be added as a regular child of the Sidebar. This keeps the sidebar generic and avoids introducing APIs that would only target a narrow use case.

Does this approach fit better with what you had in mind?

@Osaedasia Osaedasia requested a review from huacnlee December 19, 2025 21:52
@huacnlee
Copy link
Member

I'm sorry, there are many changes that look not good, I can't review to give suggestion. We need to have a good API design, not only added some feature directly.

By your original feature request, why you are changed so many APIs?

Please submit one PR that does one thing, this is important, and helps us to review your code more easily and push to merge fast.
https://github.com/longbridge/gpui-component?tab=contributing-ov-file#contributing-guide

@Osaedasia
Copy link
Author

Osaedasia commented Dec 22, 2025

Original Need

My original feature request was to have a sidebar group with a custom header (with action buttons) instead of just a text label. Following your suggestion, I created a new SidebarSection component rather than modifying SidebarGroup.

However, this introduced a type incompatibility problem: Sidebar<E> is generic over a single type E, so you cannot mix SidebarGroup<SidebarMenu> and SidebarSection<SidebarMenu> as children. This is why I added SidebarContent - it's not a feature, it's a necessary type wrapper to make both work together.

What I Added & Why

Component Purpose Necessity
SidebarSection Sidebar group with custom header element Core feature - the original request
SidebarContent Enum wrapper to mix SidebarGroup and SidebarSection in the same sidebar Required
SectionCollapsedDisplay Controls what to show when sidebar is collapsed Important - allow more customisation on what to show in SidebarSection when collapsed
Sidebar::default_content() Ergonomic constructor that avoids verbose syntax Optional convenience
.icon() Shorthand to add an icon before the header Optional convenience
.action() Shortand to add an action button Optional convenience
.loading() Show skeleton placeholders Optional
.empty_state() Show placeholders when no children Optional
.separator_before/after() Visual separators Optional

Detailed Documentation

1. SidebarSection

A sidebar group that accepts a custom header element instead of a text label.

SidebarGroup::new("Connections")
    .child(SidebarMenu::new())

SidebarSection::new()
    .header(
        div().flex().justify_between().flex_1()
            .child("Connections")
            .child(Button::new("add").icon(IconName::Plus).ghost().xsmall())
    )
    .child(SidebarMenu::new())

2. SidebarContent

Sidebar requires all children to be the same type. This enum allows mixing both group types.

// Compile error - mismatched types
Sidebar::new(Side::Left)
    .child(SidebarGroup::new("Platform").child(SidebarMenu::new()))      // SidebarGroup<SidebarMenu>
    .child(SidebarSection::new().header(...).child(SidebarMenu::new()))  // SidebarSection<SidebarMenu>

Sidebar::<SidebarContent<SidebarMenu>>::new(Side::Left)
    .child(SidebarGroup::new("Platform").child(SidebarMenu::new()).into())
    .child(SidebarSection::new().header(...).child(SidebarMenu::new()).into())

3. Sidebar::default_content()

Ergonomic constructoir for the common case of mixing SidebarGroup and SidebarSection.

// Verbose syntax
Sidebar::<SidebarContent<SidebarMenu>>::new(Side::Left)
    .child(SidebarGroup::new("Platform").child(...).into())
    .child(SidebarSection::new().header(...).into())

// Cleaner with default_content()
Sidebar::default_content(Side::Left)
    .child(SidebarGroup::new("Platform").child(...).into())
    .child(SidebarSection::new().header(...).into())

4. SectionCollapsedDisplay

When the sidebar collapses, SidebarGroup hides its label and show the childrens. But SidebarSection has richer content (icons, actions), users need control over what remains visible.

pub enum SectionCollapsedDisplay {
    Hidden,    // Show nothing
    Icon,      // Show only the section icon
    Header,    // Show the header
    Action,    // Show only the action button
    Children,  // Show children in collapsed mode (default)  (like SidebarGroup)
    Custom(AnyElement), // Custom collapsed representation
}

SidebarSection::new()
    .icon(IconName::Database)
    .header("Connections")
    .collapsed_header(SectionCollapsedDisplay::Icon)

What I Can Remove

If the PR is too large, I can remove these optional convenience methods:

  • .icon() - users can include in .header() manually
  • .action() - users can include in .header() manually
  • .loading() - not essential
  • .empty_state() - not essential
  • .separator_before/after() - not essential
  • Sidebar::default_content() - users can use verbose syntax

What I Can Remove

These arre essential for the feature to work:

  • SidebarSection with .header()
  • SidebarContent enum
  • SectionCollapsedDisplay enum

Shoud I submit a smaller PR with only the essential components, and add the convenience methods in follow-up PRs ?

Note: I'm still learning Rust, and my solution with SidebarContent enum to mix different types might not be the most idiomatic approach, it's just the only way I found to work around Rust's type system constraints. If you know a better pattern to solve this problem, I'd be very interested to learn!

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.

2 participants