Feat/navbar vertical next inline collapse#2087
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces an 'inline collapse' mode for the SiNavbarVerticalNextComponent, which replaces the collapsed navbar with a compact control bar. The implementation adds an inlineCollapse input signal, updates the component's template and SCSS for the new layout, and integrates layout-specific icons. Additionally, the PR includes unit and E2E tests to ensure correct behavior and visual consistency, along with updates to example pages. I have no feedback to provide.
mistrykaran91
force-pushed
the
feat/navbar-vertical-next-inline-collapse
branch
from
ffa39d0 to
f985be6
Compare
mistrykaran91
force-pushed
the
feat/navbar-vertical-next-inline-collapse
branch
from
aae8515 to
3a4c6ef
Compare
mistrykaran91
force-pushed
the
feat/navbar-vertical-next-inline-collapse
branch
from
3a4c6ef to
5e14e6e
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces an 'inline collapse' mode for the 'SiNavbarVerticalNextComponent', allowing the navbar to collapse into a compact control bar. The changes include adding the 'inlineCollapse' input, updating the component's template and styles to handle the new layout, and implementing focus management for the toggle buttons. Corresponding unit tests and example updates have been provided to support this new feature. I have no feedback to provide as the implementation appears complete and well-tested.
mistrykaran91
force-pushed
the
feat/navbar-vertical-next-inline-collapse
branch
from
5e14e6e to
edc50b3
Compare
mistrykaran91
force-pushed
the
feat/navbar-vertical-next-inline-collapse
branch
2 times, most recently
from
cb080d8 to
d785546
Compare
mistrykaran91
force-pushed
the
feat/navbar-vertical-next-inline-collapse
branch
3 times, most recently
from
12502fc to
5ad5c17
Compare
- The collapse/expand icons are switched to elementLayoutPane2 / elementLayoutPane2Right when `inlineCollapse` mode - Updated example components and templates to include a control for inlineCollapse. Related #1945
mistrykaran91
force-pushed
the
feat/navbar-vertical-next-inline-collapse
branch
from
5ad5c17 to
c233bdc
Compare
|
Going with the one button approach and PR for the same - #2097 |
| } | ||
|
|
||
| .inline-collapse-toggle { | ||
| display: block; |
There was a problem hiding this comment.
icon is not properly aligned vertically center
| <div class="collapse-toggle ms-auto"> | ||
| <div class="mobile-drawer focus-inside navbar-vertical-no-collapse"> | ||
| <button | ||
| #navToggleButton |
There was a problem hiding this comment.
hover state is not working once in expanded view
| } | ||
|
|
||
| :host(.nav-inline-collapse) { | ||
| nav { |
There was a problem hiding this comment.
main header should adjust its left margin to vertically align with rest of the content in inlineToggle mode
| .inline-collapse-toggle { | ||
| display: block; | ||
| margin-block-start: map.get(all-variables.$spacers, 6); | ||
| margin-inline-start: map.get(all-variables.$spacers, 9); |
There was a problem hiding this comment.
margin inline shouldn't always be 32px but it should change to 16px based on breakpoint logic same as si-layout-main-padding
2 participants
elementLayoutPane2Right in case of inlineCollapse mode
Related #1945
Documentation.
Examples.
Dashboards Demo.
Playwright report.
Coverage Reports: