Skip to content

Commit 40303e1

Browse files
authored
Move the filled star icon for feedback widget from python code to web app (#9097)
## Describe your changes Merge after #9094 We want to move the icon so that the `ButtonGroup.tsx` component does not allow arbitrary HTML strings for security reasons, but only parses the filled star icon as such a string. ## GitHub Issue Link (if applicable) ## Testing Plan - Updates the tests according to the new helper functions --- **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 6296baf commit 40303e1

File tree

7 files changed

+48
-67
lines changed

7 files changed

+48
-67
lines changed

frontend/lib/src/components/shared/Icon/DynamicIcon.test.tsx

+13-9
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,11 @@ import { render } from "@streamlit/lib/src/test_util"
1919
import { screen } from "@testing-library/react"
2020
import "@testing-library/jest-dom"
2121

22-
import { DynamicIcon, DynamicIconProps, isMaterialIcon } from "./DynamicIcon"
22+
import {
23+
DynamicIcon,
24+
DynamicIconProps,
25+
getFilledStarIconSrc,
26+
} from "./DynamicIcon"
2327

2428
const getProps = (
2529
props: Partial<DynamicIconProps> = {}
@@ -51,13 +55,13 @@ describe("Dynamic icon", () => {
5155
expect(testId.textContent).toEqual(icon.textContent)
5256
})
5357

54-
it("isMaterialIcon returns correct results", () => {
55-
expect(isMaterialIcon(":material/test:")).toBeTruthy()
56-
expect(isMaterialIcon(":material/test-hyphen:")).toBeTruthy()
57-
expect(isMaterialIcon(":material/test_underscore:")).toBeTruthy()
58-
expect(isMaterialIcon(":material/test")).toBeFalsy()
59-
expect(isMaterialIcon("material/test:")).toBeFalsy()
60-
expect(isMaterialIcon("material/test")).toBeFalsy()
61-
expect(isMaterialIcon(":materialtest:")).toBeFalsy()
58+
it("renders without crashing Styled image", () => {
59+
const props = getProps({ iconValue: ":material/star_filled:" })
60+
render(<DynamicIcon {...props} />)
61+
const testId = screen.getByTestId("stImageIcon")
62+
const srcAttr = testId.getAttribute("src")
63+
64+
expect(testId).toBeInTheDocument()
65+
expect(srcAttr).toEqual(getFilledStarIconSrc())
6266
})
6367
})

frontend/lib/src/components/shared/Icon/DynamicIcon.tsx

+24-10
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,7 @@ import React, { Suspense } from "react"
1919
import { IconSize, ThemeColor } from "@streamlit/lib/src/theme"
2020
import { EmojiIcon } from "./Icon"
2121
import MaterialFontIcon from "./Material/MaterialFontIcon"
22-
import { StyledDynamicIcon } from "./styled-components"
22+
import { StyledDynamicIcon, StyledImageIcon } from "./styled-components"
2323

2424
interface IconPackEntry {
2525
pack: string
@@ -39,10 +39,12 @@ function parseIconPackEntry(iconName: string): IconPackEntry {
3939
return { pack: iconPack, icon: iconNameInPack }
4040
}
4141

42-
export function isMaterialIcon(option: string): boolean {
43-
const materialIconRegexp = /^:material\/(.+):$/
44-
const materialIconMatch = materialIconRegexp.exec(option)
45-
return materialIconMatch !== null
42+
/**
43+
*
44+
* @returns returns an img tag with a yellow filled star icon svg as base64 data
45+
*/
46+
export function getFilledStarIconSrc(): string {
47+
return "data:image/svg+xml;base64,PHN2ZyB3aWR0aD0iMjAiIGhlaWdodD0iMjAiIHZpZXdCb3g9IjAgMCAyMCAyMCIgZmlsbD0ibm9uZSIgeG1sbnM9Imh0dHA6Ly93d3cudzMub3JnLzIwMDAvc3ZnIj48ZyBjbGlwLXBhdGg9InVybCgjY2xpcDBfMTg2MF84NDMpIj48cGF0aCBkPSJNOS45OTk5NCAxNC4zOTE2TDEzLjQ1ODMgMTYuNDgzM0MxNC4wOTE2IDE2Ljg2NjYgMTQuODY2NiAxNi4zIDE0LjY5OTkgMTUuNTgzM0wxMy43ODMzIDExLjY1TDE2Ljg0MTYgOC45OTk5N0MxNy4zOTk5IDguNTE2NjMgMTcuMDk5OSA3LjU5OTk3IDE2LjM2NjYgNy41NDE2M0wxMi4zNDE2IDcuMTk5OTdMMTAuNzY2NiAzLjQ4MzNDMTAuNDgzMyAyLjgwODMgOS41MTY2MSAyLjgwODMgOS4yMzMyNyAzLjQ4MzNMNy42NTgyNyA3LjE5MTYzTDMuNjMzMjcgNy41MzMzQzIuODk5OTQgNy41OTE2MyAyLjU5OTk0IDguNTA4MyAzLjE1ODI3IDguOTkxNjNMNi4yMTY2MSAxMS42NDE2TDUuMjk5OTQgMTUuNTc1QzUuMTMzMjcgMTYuMjkxNiA1LjkwODI3IDE2Ljg1ODMgNi41NDE2MSAxNi40NzVMOS45OTk5NCAxNC4zOTE2WiIgZmlsbD0iI0ZBQ0EyQiIvPjwvZz48ZGVmcz48Y2xpcFBhdGggaWQ9ImNsaXAwXzE4NjBfODQzIj48cmVjdCB3aWR0aD0iMjAiIGhlaWdodD0iMjAiIGZpbGw9IndoaXRlIi8+PC9jbGlwUGF0aD48L2RlZnM+PC9zdmc+"
4648
}
4749

4850
export interface DynamicIconProps {
@@ -61,11 +63,23 @@ const DynamicIconDispatcher = ({
6163
const { pack, icon } = parseIconPackEntry(iconValue)
6264
switch (pack) {
6365
case "material":
64-
return (
65-
<StyledDynamicIcon {...props}>
66-
<MaterialFontIcon pack={pack} iconName={icon} {...props} />
67-
</StyledDynamicIcon>
68-
)
66+
switch (icon) {
67+
case "star_filled":
68+
return (
69+
<StyledDynamicIcon {...props}>
70+
<StyledImageIcon
71+
src={getFilledStarIconSrc()}
72+
data-testid={props.testid || "stImageIcon"}
73+
/>
74+
</StyledDynamicIcon>
75+
)
76+
default:
77+
return (
78+
<StyledDynamicIcon {...props}>
79+
<MaterialFontIcon pack={pack} iconName={icon} {...props} />
80+
</StyledDynamicIcon>
81+
)
82+
}
6983
case "emoji":
7084
default:
7185
return (

frontend/lib/src/components/shared/Icon/index.tsx

+1-1
Original file line numberDiff line numberDiff line change
@@ -15,5 +15,5 @@
1515
*/
1616

1717
export { default, EmojiIcon } from "./Icon"
18-
export { DynamicIcon, isMaterialIcon } from "./DynamicIcon"
18+
export { DynamicIcon, getFilledStarIconSrc } from "./DynamicIcon"
1919
export { StyledIcon, StyledSpinnerIcon } from "./styled-components"

frontend/lib/src/components/shared/Icon/styled-components.ts

+7
Original file line numberDiff line numberDiff line change
@@ -105,6 +105,13 @@ export const StyledDynamicIcon = styled.span<StyledDynamicIconProps>(
105105
}
106106
)
107107

108+
export const StyledImageIcon = styled.img(({}) => {
109+
return {
110+
width: "100%",
111+
height: "100%",
112+
}
113+
})
114+
108115
interface StyledEmojiIconProps {
109116
size: IconSize
110117
margin: string

frontend/lib/src/components/widgets/ButtonGroup/ButtonGroup.test.tsx

-21
Original file line numberDiff line numberDiff line change
@@ -102,27 +102,6 @@ describe("ButtonGroup widget", () => {
102102
})
103103
})
104104

105-
it("option-children with markdown render correctly", () => {
106-
const markdownOptions = [
107-
ButtonGroupProto.Option.create({ content: "Some text" }),
108-
ButtonGroupProto.Option.create({
109-
content: "Some text 2",
110-
}),
111-
]
112-
const props = getProps({ options: markdownOptions })
113-
render(<ButtonGroup {...props} />)
114-
115-
const buttonGroupWidget = screen.getByTestId("stButtonGroup")
116-
const buttons = within(buttonGroupWidget).getAllByRole("button")
117-
expect(buttons).toHaveLength(2)
118-
buttons.forEach(button => {
119-
expect(button).toHaveAttribute("kind", "borderlessIcon")
120-
within(button).getByTestId("stMarkdownContainer")
121-
})
122-
expect(buttons[0].textContent).toContain("Some text")
123-
expect(buttons[1].textContent).toContain("Some text 2")
124-
})
125-
126105
it("sets widget value on mount", () => {
127106
const props = getProps()
128107
jest.spyOn(props.widgetMgr, "setIntArrayValue")

frontend/lib/src/components/widgets/ButtonGroup/ButtonGroup.tsx

+2-22
Original file line numberDiff line numberDiff line change
@@ -32,16 +32,11 @@ import BaseButton, {
3232
BaseButtonKind,
3333
BaseButtonSize,
3434
} from "@streamlit/lib/src/components/shared/BaseButton"
35-
import {
36-
DynamicIcon,
37-
isMaterialIcon,
38-
} from "@streamlit/lib/src/components/shared/Icon"
35+
import { DynamicIcon } from "@streamlit/lib/src/components/shared/Icon"
3936
import { EmotionTheme } from "@streamlit/lib/src/theme"
4037

4138
import { ButtonGroup as ButtonGroupProto } from "@streamlit/lib/src/proto"
4239
import { WidgetStateManager } from "@streamlit/lib/src/WidgetStateManager"
43-
import StreamlitMarkdown from "@streamlit/lib/src/components/shared/StreamlitMarkdown"
44-
import { iconSizes } from "@streamlit/lib/src/theme/primitives"
4540
import { FormClearHelper } from "@streamlit/lib/src/components/widgets/Form/FormClearHelper"
4641

4742
export interface Props {
@@ -90,22 +85,7 @@ function syncWithWidgetManager(
9085
}
9186

9287
function getContentElement(content: string): ReactElement {
93-
const fontSize = "lg"
94-
if (isMaterialIcon(content)) {
95-
return <DynamicIcon size={fontSize} iconValue={content} />
96-
}
97-
98-
return (
99-
<StreamlitMarkdown
100-
source={content}
101-
allowHTML={true}
102-
style={{
103-
marginBottom: 0,
104-
width: iconSizes[fontSize],
105-
display: "inline-flex",
106-
}}
107-
/>
108-
)
88+
return <DynamicIcon size="lg" iconValue={content} />
10989
}
11090

11191
/**

lib/streamlit/elements/widgets/button_group.py

+1-4
Original file line numberDiff line numberDiff line change
@@ -78,10 +78,7 @@
7878
_STAR_ICON: Final = ":material/star:"
7979
# we don't have the filled-material icon library as a dependency. Hence, we have it here
8080
# in base64 format and send it over the wire as an image.
81-
_SELECTED_STAR_ICON: Final = (
82-
"<img src='data:image/svg+xml;base64,"
83-
"PHN2ZyB3aWR0aD0iMjAiIGhlaWdodD0iMjAiIHZpZXdCb3g9IjAgMCAyMCAyMCIgZmlsbD0ibm9uZSIgeG1sbnM9Imh0dHA6Ly93d3cudzMub3JnLzIwMDAvc3ZnIj48ZyBjbGlwLXBhdGg9InVybCgjY2xpcDBfMTg2MF84NDMpIj48cGF0aCBkPSJNOS45OTk5NCAxNC4zOTE2TDEzLjQ1ODMgMTYuNDgzM0MxNC4wOTE2IDE2Ljg2NjYgMTQuODY2NiAxNi4zIDE0LjY5OTkgMTUuNTgzM0wxMy43ODMzIDExLjY1TDE2Ljg0MTYgOC45OTk5N0MxNy4zOTk5IDguNTE2NjMgMTcuMDk5OSA3LjU5OTk3IDE2LjM2NjYgNy41NDE2M0wxMi4zNDE2IDcuMTk5OTdMMTAuNzY2NiAzLjQ4MzNDMTAuNDgzMyAyLjgwODMgOS41MTY2MSAyLjgwODMgOS4yMzMyNyAzLjQ4MzNMNy42NTgyNyA3LjE5MTYzTDMuNjMzMjcgNy41MzMzQzIuODk5OTQgNy41OTE2MyAyLjU5OTk0IDguNTA4MyAzLjE1ODI3IDguOTkxNjNMNi4yMTY2MSAxMS42NDE2TDUuMjk5OTQgMTUuNTc1QzUuMTMzMjcgMTYuMjkxNiA1LjkwODI3IDE2Ljg1ODMgNi41NDE2MSAxNi40NzVMOS45OTk5NCAxNC4zOTE2WiIgZmlsbD0iI0ZBQ0EyQiIvPjwvZz48ZGVmcz48Y2xpcFBhdGggaWQ9ImNsaXAwXzE4NjBfODQzIj48cmVjdCB3aWR0aD0iMjAiIGhlaWdodD0iMjAiIGZpbGw9IndoaXRlIi8+PC9jbGlwUGF0aD48L2RlZnM+PC9zdmc+'/>"
84-
)
81+
_SELECTED_STAR_ICON: Final = ":material/star_filled:"
8582

8683
_FeedbackOptions: TypeAlias = Literal["thumbs", "faces", "stars"]
8784

0 commit comments

Comments
 (0)