Skip to content

Commit

Permalink
Add expanded param to st.navigation (streamlit#9484)
Browse files Browse the repository at this point in the history
## Describe your changes

This change does two things:

1. Add expanded param to `st.navigation` that will move the navigation
to expanded
2. Preserve user's preference when the View More button is clicked.
Remove the preference when clicking View Less.

Two notable behaviors occur that seem reasonable:

1. Setting `expanded=True` _removes_ the View More Button, effectively
disallowing collapse. This is due to a conflict from the script run and
the user behavior.
2. If `expanded=True` and then `expanded=False`, the nav will default to
expanded (with the View Less Button re-appearing).


## GitHub Issue Link (if applicable)

## Testing Plan

- Explanation of why no additional tests are needed
- Unit Tests (JS and/or Python)
- E2E Tests
- Any manual testing needed?

---

**Contribution License Agreement**

By submitting this pull request you agree that all contributions to this
project are made under the Apache 2.0 license.
  • Loading branch information
kmcgrady authored Sep 18, 2024
1 parent 67aaff5 commit 5622102
Show file tree
Hide file tree
Showing 14 changed files with 321 additions and 19 deletions.
2 changes: 2 additions & 0 deletions e2e_playwright/multipage_apps_v2/mpa_v2_basics.py
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@ def fragment_number_input():

hide_sidebar = st.checkbox("Hide sidebar")
dynamic_nav = st.checkbox("Change navigation dynamically")
expanded = st.checkbox("Expand navigation")
pg = st.navigation(
(
[page2, page3, page5, page9]
Expand All @@ -102,6 +103,7 @@ def fragment_number_input():
}
),
position="hidden" if hide_sidebar else "sidebar",
expanded=expanded,
)

if st.button("page 5"):
Expand Down
87 changes: 87 additions & 0 deletions e2e_playwright/multipage_apps_v2/mpa_v2_basics_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -218,6 +218,93 @@ def test_handles_expand_collapse_of_mpa_nav_correctly(
)


def test_handles_expanded_navigation_parameter_correctly(app: Page):
"""Test that we handle expanded param of st.navigation nav correctly."""

click_checkbox(app, "Show sidebar elements")
wait_for_app_run(app)

# By default, the navigation is collapsed
view_button = app.get_by_test_id("stSidebarNavViewButton")
expect(view_button).to_be_visible()

links = app.get_by_test_id("stSidebarNav").locator("a")
expect(links).to_have_count(10)

# Forced expansion removes the View less button and shows all links
click_checkbox(app, "Expand navigation")
wait_for_app_run(app)

view_button = app.get_by_test_id("stSidebarNavViewButton")

expect(view_button).not_to_be_visible()
links = app.get_by_test_id("stSidebarNav").locator("a")
expect(links).to_have_count(13)

# Removing forced expansion shows the View less button but remains expanded
click_checkbox(app, "Expand navigation")
wait_for_app_run(app)
view_button = app.get_by_test_id("stSidebarNavViewButton")

expect(view_button).to_be_visible()
links = app.get_by_test_id("stSidebarNav").locator("a")
expect(links).to_have_count(13)


def test_preserves_navigation_expansion_user_preference(app: Page, app_port: int):
"""Test that the navigation expansion state is preserved across page changes."""
click_checkbox(app, "Show sidebar elements")
wait_for_app_run(app)

# verify the default setting is collapsed
view_more_button = app.get_by_test_id("stSidebarNavViewButton")
expect(view_more_button).to_be_visible()
links = app.get_by_test_id("stSidebarNav").locator("a")
expect(links).to_have_count(10)

# User clicks View more which preserves the setting
view_more_button.click()

# Verify navigation is expanded
view_less_button = app.get_by_test_id("stSidebarNavViewButton")
expect(view_less_button).to_have_text("View less")
links = app.get_by_test_id("stSidebarNav").locator("a")
expect(links).to_have_count(13)

# Reload the page and ensure elements are in the sidebar
app.goto(f"http://localhost:{app_port}")
wait_for_app_loaded(app)

click_checkbox(app, "Show sidebar elements")
wait_for_app_run(app)

# Verify navigation remains expanded
links = app.get_by_test_id("stSidebarNav").locator("a")
expect(links).to_have_count(13)
view_less_button = app.get_by_test_id("stSidebarNavViewButton")
expect(view_less_button).to_have_text("View less")

# Undo the setting (eliminating the preference)
view_less_button.click()

# Verify navigation is collapsed
view_less_button = app.get_by_test_id("stSidebarNavViewButton")
expect(view_less_button).to_have_text("View 3 more")
links = app.get_by_test_id("stSidebarNav").locator("a")
expect(links).to_have_count(10)

# Reload the page and ensure elements are in the sidebar
app.goto(f"http://localhost:{app_port}")
wait_for_app_loaded(app)

click_checkbox(app, "Show sidebar elements")
wait_for_app_run(app)

links = app.get_by_test_id("stSidebarNav").locator("a")
expect(links).to_have_count(10)
expect(app.get_by_test_id("stSidebarNavViewButton")).to_have_text("View 3 more")


def test_switch_page_by_path(app: Page):
"""Test that we can switch between pages by triggering st.switch_page with a path."""

Expand Down
4 changes: 4 additions & 0 deletions frontend/app/src/App.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,7 @@ interface State {
formsData: FormsData
hideTopBar: boolean
hideSidebarNav: boolean
expandSidebarNav: boolean
appPages: IAppPage[]
navSections: string[]
// The hash of the current page executing
Expand Down Expand Up @@ -293,6 +294,7 @@ export class App extends PureComponent<Props, State> {
// true as well for consistency.
hideTopBar: true,
hideSidebarNav: true,
expandSidebarNav: false,
toolbarMode: Config.ToolbarMode.MINIMAL,
latestRunTime: performance.now(),
fragmentIdsThisRun: [],
Expand Down Expand Up @@ -1821,6 +1823,7 @@ export class App extends PureComponent<Props, State> {
userSettings,
hideTopBar,
hideSidebarNav,
expandSidebarNav,
currentPageScriptHash,
hostHideSidebarNav,
pageLinkBaseUrl,
Expand Down Expand Up @@ -1974,6 +1977,7 @@ export class App extends PureComponent<Props, State> {
onPageChange={this.onPageChange}
currentPageScriptHash={currentPageScriptHash}
hideSidebarNav={hideSidebarNav || hostHideSidebarNav}
expandSidebarNav={expandSidebarNav}
/>
{renderedDialog}
</StyledApp>
Expand Down
1 change: 1 addition & 0 deletions frontend/app/src/components/AppView/AppView.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,7 @@ function getProps(props: Partial<AppViewProps> = {}): AppViewProps {
onPageChange: jest.fn(),
currentPageScriptHash: "main_page_script_hash",
hideSidebarNav: false,
expandSidebarNav: false,
...props,
}
}
Expand Down
4 changes: 4 additions & 0 deletions frontend/app/src/components/AppView/AppView.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,8 @@ export interface AppViewProps {
currentPageScriptHash: string

hideSidebarNav: boolean

expandSidebarNav: boolean
}

/**
Expand All @@ -112,6 +114,7 @@ function AppView(props: AppViewProps): ReactElement {
navSections,
onPageChange,
currentPageScriptHash,
expandSidebarNav,
hideSidebarNav,
sendMessageToHost,
endpoints,
Expand Down Expand Up @@ -246,6 +249,7 @@ function AppView(props: AppViewProps): ReactElement {
onPageChange={onPageChange}
currentPageScriptHash={currentPageScriptHash}
hideSidebarNav={hideSidebarNav}
expandSidebarNav={expandSidebarNav}
>
<StyledSidebarBlockContainer>
{renderBlock(elements.sidebar)}
Expand Down
1 change: 1 addition & 0 deletions frontend/app/src/components/Sidebar/Sidebar.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ function renderSidebar(props: Partial<SidebarProps> = {}): RenderResult {
currentPageScriptHash={""}
hasElements
hideSidebarNav={false}
expandSidebarNav={false}
{...props}
/>
)
Expand Down
3 changes: 3 additions & 0 deletions frontend/app/src/components/Sidebar/Sidebar.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ export interface SidebarProps {
onPageChange: (pageName: string) => void
currentPageScriptHash: string
hideSidebarNav: boolean
expandSidebarNav: boolean
}

interface State {
Expand Down Expand Up @@ -279,6 +280,7 @@ class Sidebar extends PureComponent<SidebarProps, State> {
onPageChange,
currentPageScriptHash,
hideSidebarNav,
expandSidebarNav,
navSections,
} = this.props

Expand Down Expand Up @@ -368,6 +370,7 @@ class Sidebar extends PureComponent<SidebarProps, State> {
currentPageScriptHash={currentPageScriptHash}
navSections={navSections}
hasSidebarElements={hasElements}
expandSidebarNav={expandSidebarNav}
onPageChange={onPageChange}
/>
)}
Expand Down
67 changes: 66 additions & 1 deletion frontend/app/src/components/Sidebar/SidebarNav.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ const getProps = (props: Partial<Props> = {}): Props => ({
collapseSidebar: jest.fn(),
currentPageScriptHash: "",
hasSidebarElements: false,
expandSidebarNav: false,
onPageChange: jest.fn(),
endpoints: mockEndpoints(),
...props,
Expand All @@ -57,6 +58,7 @@ describe("SidebarNav", () => {
afterEach(() => {
// @ts-expect-error
reactDeviceDetect.isMobile = false
window.localStorage.clear()
})

it("replaces underscores with spaces in pageName", () => {
Expand Down Expand Up @@ -147,6 +149,37 @@ describe("SidebarNav", () => {
)
})

it("does not render View less button when explicitly asked to expand", () => {
render(
<SidebarNav
{...getProps({
expandSidebarNav: true,
hasSidebarElements: true,
appPages: [
{
pageScriptHash: "main_page_hash",
pageName: "streamlit app",
urlPathname: "streamlit_app",
isDefault: true,
},
].concat(
Array.from({ length: 12 }, (_, index) => ({
pageScriptHash: `other_page_hash${index}`,
pageName: `my other page${index}`,
urlPathname: `my_other_page${index}`,
isDefault: false,
}))
),
})}
/>
)

expect(screen.getByTestId("stSidebarNavSeparator")).toBeInTheDocument()
expect(
screen.queryByTestId("stSidebarNavViewButton")
).not.toBeInTheDocument()
})

it("renders View more button when there are more than 13 elements", () => {
render(
<SidebarNav
Expand Down Expand Up @@ -207,7 +240,7 @@ describe("SidebarNav", () => {
expect(screen.getAllByTestId("stSidebarNavLink")).toHaveLength(12)
})

it("renders View less button when visible and expanded", async () => {
it("renders View less button when expanded", async () => {
render(
<SidebarNav
{...getProps({
Expand Down Expand Up @@ -238,6 +271,38 @@ describe("SidebarNav", () => {
expect(viewLessButton).toBeInTheDocument()
})

it("renders View less button when user prefers expansion", () => {
window.localStorage.setItem("sidebarNavState", "expanded")

render(
<SidebarNav
{...getProps({
hasSidebarElements: true,
appPages: [
{
pageScriptHash: "main_page_hash",
pageName: "streamlit app",
urlPathname: "streamlit_app",
isDefault: true,
},
].concat(
Array.from({ length: 13 }, (_, index) => ({
pageScriptHash: `other_page_hash${index}`,
pageName: `my other page${index}`,
urlPathname: `my_other_page${index}`,
isDefault: false,
}))
),
})}
/>
)

const viewLessButton = screen.getByText("View less")
expect(viewLessButton).toBeInTheDocument()
const navLinks = screen.getAllByTestId("stSidebarNavLink")
expect(navLinks).toHaveLength(14)
})

it("is unexpanded by default, displaying 10 links when > 12 pages", () => {
render(
<SidebarNav
Expand Down
31 changes: 28 additions & 3 deletions frontend/app/src/components/Sidebar/SidebarNav.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import React, {
ReactNode,
useCallback,
useContext,
useEffect,
useState,
} from "react"

Expand All @@ -28,7 +29,11 @@ import groupBy from "lodash/groupBy"
// isMobile field sanely.
import * as reactDeviceDetect from "react-device-detect"

import { IAppPage, StreamlitEndpoints } from "@streamlit/lib"
import {
IAppPage,
localStorageAvailable,
StreamlitEndpoints,
} from "@streamlit/lib"
import { AppContext } from "@streamlit/app/src/components/AppContext"

import NavSection from "./NavSection"
Expand All @@ -47,6 +52,7 @@ export interface Props {
collapseSidebar: () => void
currentPageScriptHash: string
hasSidebarElements: boolean
expandSidebarNav: boolean
onPageChange: (pageName: string) => void
}

Expand Down Expand Up @@ -132,6 +138,7 @@ const SidebarNav = ({
endpoints,
appPages,
collapseSidebar,
expandSidebarNav,
currentPageScriptHash,
hasSidebarElements,
navSections,
Expand All @@ -140,8 +147,26 @@ const SidebarNav = ({
const [expanded, setExpanded] = useState(false)
const { pageLinkBaseUrl } = useContext(AppContext)

useEffect(() => {
const cachedSidebarNavExpanded =
localStorageAvailable() &&
window.localStorage.getItem("sidebarNavState") === "expanded"

if (!expanded && (expandSidebarNav || cachedSidebarNavExpanded)) {
setExpanded(true)
}
}, [expanded, expandSidebarNav])

const handleViewButtonClick = useCallback(() => {
setExpanded(!expanded)
const nextState = !expanded
if (localStorageAvailable()) {
if (nextState) {
localStorage.setItem("sidebarNavState", "expanded")
} else {
localStorage.removeItem("sidebarNavState")
}
}
setExpanded(nextState)
}, [expanded])

const generateNavLink = useCallback(
Expand Down Expand Up @@ -177,7 +202,7 @@ const SidebarNav = ({
let contents: ReactNode[] = []
const totalPages = appPages.length
const shouldShowViewButton =
hasSidebarElements && totalPages > COLLAPSE_THRESHOLD
hasSidebarElements && totalPages > COLLAPSE_THRESHOLD && !expandSidebarNav
const needsCollapse = shouldShowViewButton && !expanded
if (navSections.length > 0) {
// For MPAv2 with headers: renders a NavSection for each header with its respective pages
Expand Down
Loading

0 comments on commit 5622102

Please sign in to comment.