fix: Adjust sidebar banner to wrap bellow the title #1819
Conversation
oliverwhite19
requested review from
dessalines,
SleeplessOne1917,
alectrocute and
jsit
as code owners
| @@ -33,7 +33,7 @@ export class SiteSidebar extends Component<SiteSidebarProps, SiteSidebarState> { | |||
| <div className="site-sidebar accordion"> | |||
| <section id="sidebarInfo" className="card border-secondary mb-3"> | |||
| <header | |||
| className="card-header d-flex align-items-center" | |||
| className="card-header d-flex align-items-center flex-wrap" | |||
| id="sidebarInfoHeader" | |||
| > | |||
| {this.siteName()} | |||
There was a problem hiding this comment.
I'd put a bit of margin bottom on the site name when the banner is displaying, but other than that you're good.
There was a problem hiding this comment.
I've conditionally added some margin on the header
|
I think rather than doing flex-wrap, these should probably just be on different rows to begin with. And I wonder if it would look better with the banner on top? |
I've set the elements on different rows and it does simplify things. I don't like the idea of putting the banner on top because then you move the collapse button up and down, which is bad UX imo |
| @@ -54,7 +51,7 @@ export class SiteSidebar extends Component<SiteSidebarProps, SiteSidebarState> { | |||
|
|
|||
| siteName() { | |||
| return ( | |||
| <> | |||
| <div className={!this.state.collapsed ? "mb-2" : undefined}> | |||
There was a problem hiding this comment.
If the 2nd condition is empty, just do !this.state.collapsed && "mb-2" , or ever better, use the classNames util.
There was a problem hiding this comment.
!this.state.collapsed && "mb-2" gives a typescript error because it doesn't like passing a boolean to className, but the classNames util works great, thanks for telling me about it!
Description
Fixes #1607
Makes it so that the sidebar and banner don't appear in the same row.
Screenshots
Before
After