Skip to content

Commit

Permalink
Fix initial iframe height for custom component (#8290)
Browse files Browse the repository at this point in the history
## Describe your changes

In 1.32.0, we introduced some changes to custom components that rendered
an iframe of a certain height instead of a default of 0. This change
ensures that when the `frameHeight` is `undefined`, it will use 0.

## GitHub Issue Link (if applicable)
Closes #8285

## Testing Plan

- Added a test to make sure it's set (and the Skeleton appears and
disappears)

---

**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 committed Mar 14, 2024
1 parent aa53d4f commit 8de2044
Show file tree
Hide file tree
Showing 2 changed files with 98 additions and 23 deletions.
Expand Up @@ -146,6 +146,54 @@ describe("ComponentInstance", () => {
expect(iframe).toHaveAttribute("sandbox", DEFAULT_IFRAME_SANDBOX_POLICY)
})

it("displays a skeleton initially with a certain height", () => {
const componentRegistry = getComponentRegistry()
render(
<ComponentInstance
element={createElementProp()}
registry={componentRegistry}
width={100}
disabled={false}
theme={mockTheme.emotion}
widgetMgr={
new WidgetStateManager({
sendRerunBackMsg: jest.fn(),
formsDataChanged: jest.fn(),
})
}
/>
)
const skeleton = screen.getByTestId("stSkeleton")
expect(skeleton).toBeInTheDocument()
expect(skeleton).toHaveStyle("height: 2.75rem")

const iframe = screen.getByTitle(MOCK_COMPONENT_NAME)
expect(iframe).toHaveAttribute("height", "0")
})

it("will not displays a skeleton when height is explicitly set to 0", () => {
const componentRegistry = getComponentRegistry()
render(
<ComponentInstance
element={createElementProp({ height: 0 })}
registry={componentRegistry}
width={100}
disabled={false}
theme={mockTheme.emotion}
widgetMgr={
new WidgetStateManager({
sendRerunBackMsg: jest.fn(),
formsDataChanged: jest.fn(),
})
}
/>
)
expect(screen.queryByTestId("stSkeleton")).not.toBeInTheDocument()

const iframe = screen.getByTitle(MOCK_COMPONENT_NAME)
expect(iframe).toHaveAttribute("height", "0")
})

describe("COMPONENT_READY handler", () => {
it("posts a RENDER message to the iframe", () => {
const jsonArgs = { foo: "string", bar: 5 }
Expand Down Expand Up @@ -184,6 +232,43 @@ describe("ComponentInstance", () => {
expect(postMessage).toHaveBeenCalledWith(renderMsg(jsonArgs, []), "*")
})

it("hides the skeleton and maintains iframe height of 0", () => {
const componentRegistry = getComponentRegistry()
render(
<ComponentInstance
element={createElementProp()}
registry={componentRegistry}
width={100}
disabled={false}
theme={mockTheme.emotion}
widgetMgr={
new WidgetStateManager({
sendRerunBackMsg: jest.fn(),
formsDataChanged: jest.fn(),
})
}
/>
)

const iframe = screen.getByTitle(MOCK_COMPONENT_NAME)

// SET COMPONENT_READY
fireEvent(
window,
new MessageEvent("message", {
data: {
isStreamlitMessage: true,
apiVersion: 1,
type: ComponentMessageType.COMPONENT_READY,
},
// @ts-expect-error
source: iframe.contentWindow,
})
)
expect(screen.queryByTestId("stSkeleton")).not.toBeInTheDocument()
expect(iframe).toHaveAttribute("height", "0")
})

it("prevents RENDER message until component is ready", () => {
const jsonArgs = { foo: "string", bar: 5 }
const componentRegistry = getComponentRegistry()
Expand Down
Expand Up @@ -119,24 +119,6 @@ function getWarnMessage(componentName: string, url?: string): string {
return message
}

type HeightUnit = "px"
/**
* Parse the height as a string and add the unit as a suffix, e.g. `(42) => '42px'`
*
* @param height height number
* @returns the height number as a string with suffix or `undefined` if `height` is `undefined`
*/
function parseToStringWithUnitSuffix(
height?: number,
unit: HeightUnit = "px"
): string | undefined {
if (!height) {
return undefined
}

return `${height}${unit}`
}

function tryParseArgs(
jsonArgs: string,
specialArgs: ISpecialArg[],
Expand Down Expand Up @@ -215,7 +197,8 @@ function ComponentInstance(props: Props): ReactElement {
const [isReadyTimeout, setIsReadyTimeout] = useState<boolean>()
// By passing the args.height here, we can derive the initial height for
// custom components that define a height property, e.g. in Python
// my_custom_component(height=100)
// my_custom_component(height=100). undefined means no explicit height
// was specified, but will be set to the default height of 0.
const [frameHeight, setFrameHeight] = useState<number | undefined>(
isNaN(parsedNewArgs.height) ? undefined : parsedNewArgs.height
)
Expand Down Expand Up @@ -351,9 +334,15 @@ function ComponentInstance(props: Props): ReactElement {

// Show the loading Skeleton while we have not received the ready message from the custom component
// but while we also have not waited until the ready timeout
const loadingSkeleton = !isReadyRef.current && !isReadyTimeout && (
<Skeleton height={parseToStringWithUnitSuffix(frameHeight)} />
)
const loadingSkeleton = !isReadyRef.current &&
!isReadyTimeout &&
// if height is explicitly set to 0, we don’t want to show the skeleton at all
frameHeight !== 0 && (
// Skeletons will have a default height if no frameHeight was specified
<Skeleton
height={frameHeight === undefined ? undefined : `${frameHeight}px`}
/>
)

// If we've timed out waiting for the READY message from the component,
// display a warning.
Expand Down Expand Up @@ -389,7 +378,8 @@ function ComponentInstance(props: Props): ReactElement {
ref={iframeRef}
src={getSrc(componentName, registry, url)}
width={width}
height={frameHeight}
// for undefined height we set the height to 0 to avoid inconsistent behavior
height={frameHeight ?? 0}
style={{
colorScheme: "light dark",
display: isReadyRef.current ? "initial" : "none",
Expand Down

0 comments on commit 8de2044

Please sign in to comment.