Skip to content

Commit 487f9cc

Browse files
sfc-gh-lwilbykajarenc
authored andcommitted
Popover with help and use_container_width is rendered correctly (#10709)
## Describe your changes This fixes an issue where `use_container_width` is ignored if the `help` parameter is passed to `st.popover` by ensuring `element.useContainerWidth` is provided to `BaseButtonTooltip` as well as `BaseButton`. ## GitHub Issue Link (if applicable) Closes #10693 ## Testing Plan Adds some playwright tests similar to the existing `use_container_width` tests but including the help option. --- **Contribution License Agreement** By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.
1 parent cd2c898 commit 487f9cc

10 files changed

+78
-2
lines changed

e2e_playwright/st_popover.py

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,13 @@
2727
with st.popover("popover 2 (use_container_width)", use_container_width=True):
2828
st.markdown("Hello")
2929

30+
with st.popover(
31+
"popover 9 (use_container_width) with help",
32+
use_container_width=True,
33+
help="help text",
34+
):
35+
st.markdown("Hello")
36+
3037
with st.popover(
3138
"popover 3 (with widgets)",
3239
):

e2e_playwright/st_popover_test.py

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,7 @@ def test_popover_button_rendering(
2727
):
2828
"""Test that the popover buttons are correctly rendered via screenshot matching."""
2929
popover_elements = themed_app.get_by_test_id("stPopover")
30-
expect(popover_elements).to_have_count(8)
30+
expect(popover_elements).to_have_count(9)
3131

3232
assert_snapshot(
3333
get_popover(themed_app, "popover 5 (in sidebar)"), name="st_popover-sidebar"
@@ -57,6 +57,10 @@ def test_popover_button_rendering(
5757
get_popover(themed_app, "popover 8 (material icon)"),
5858
name="st_popover-material_icon",
5959
)
60+
assert_snapshot(
61+
get_popover(themed_app, "popover 9 (use_container_width) with help"),
62+
name="st_popover-use_container_width_with_help",
63+
)
6064

6165

6266
def test_popover_container_rendering(
@@ -96,6 +100,17 @@ def test_popover_with_use_container_width(app: Page):
96100
expect(popover_container).to_have_css("min-width", "704px")
97101

98102

103+
def test_popover_with_use_container_width_and_help(app: Page):
104+
"""Test that the popover container is correctly stretched to the button width
105+
if `use_container_width=True` and `help` is provided."""
106+
# Get the stretched popover container:
107+
popover_container = open_popover(app, "popover 9 (use_container_width) with help")
108+
109+
expect(popover_container.get_by_test_id("stMarkdown")).to_have_text("Hello")
110+
# Check that the min width is stretched to the full container width:
111+
expect(popover_container).to_have_css("min-width", "704px")
112+
113+
99114
def test_applying_changes_from_popover_container(app: Page):
100115
"""Test that changes made in the popover container are applied correctly."""
101116
# Get the widgets popover container:

frontend/lib/src/components/elements/Popover/Popover.test.tsx

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -80,4 +80,55 @@ describe("Popover container", () => {
8080
// Text should be visible now
8181
expect(screen.queryByText("test")).toBeVisible()
8282
})
83+
84+
it("should render correctly with use_container_width and help", async () => {
85+
const user = userEvent.setup()
86+
// Hover to see tooltip content
87+
render(
88+
<Popover
89+
{...getProps({ help: "mockHelpText", useContainerWidth: true })}
90+
/>
91+
)
92+
93+
// Ensure both the button and the tooltip target have the correct width
94+
const popoverButtonWidget = screen.getByRole("button")
95+
expect(popoverButtonWidget).toHaveStyle("width: 100%")
96+
const tooltipTarget = screen.getByTestId("stTooltipHoverTarget")
97+
expect(tooltipTarget).toHaveStyle("width: 100%")
98+
99+
// Ensure the tooltip content is visible and has the correct text
100+
await user.hover(tooltipTarget)
101+
102+
const tooltipContent = await screen.findByTestId("stTooltipContent")
103+
expect(tooltipContent).toHaveTextContent("mockHelpText")
104+
})
105+
106+
it("should render correctly with help", async () => {
107+
const user = userEvent.setup()
108+
// Hover to see tooltip content
109+
render(
110+
<Popover
111+
{...getProps({ help: "mockHelpText", useContainerWidth: false })}
112+
/>
113+
)
114+
115+
// Ensure both the button and the tooltip target have the correct width
116+
const popoverButtonWidget = screen.getByRole("button")
117+
expect(popoverButtonWidget).toHaveStyle("width: auto")
118+
const tooltipTarget = screen.getByTestId("stTooltipHoverTarget")
119+
expect(tooltipTarget).toHaveStyle("width: auto")
120+
121+
// Ensure the tooltip content is visible and has the correct text
122+
await user.hover(tooltipTarget)
123+
124+
const tooltipContent = await screen.findByTestId("stTooltipContent")
125+
expect(tooltipContent).toHaveTextContent("mockHelpText")
126+
})
127+
128+
it("passes useContainerWidth property without help correctly", () => {
129+
render(<Popover {...getProps({ useContainerWidth: true })} />)
130+
131+
const popoverButtonWidget = screen.getByRole("button")
132+
expect(popoverButtonWidget).toHaveStyle("width: 100%")
133+
})
83134
})

frontend/lib/src/components/elements/Popover/Popover.tsx

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -127,7 +127,10 @@ const Popover: React.FC<React.PropsWithChildren<PopoverProps>> = ({
127127
{/* This needs to be wrapped into a div, otherwise
128128
the BaseWeb popover implementation will not work correctly. */}
129129
<div>
130-
<BaseButtonTooltip help={element.help}>
130+
<BaseButtonTooltip
131+
help={element.help}
132+
containerWidth={element.useContainerWidth}
133+
>
131134
<BaseButton
132135
data-testid="stPopoverButton"
133136
kind={BaseButtonKind.SECONDARY}

0 commit comments

Comments
 (0)