Skip to content

Commit

Permalink
Refactor frontend styles, class names and test-ids - Part 1 (streamli…
Browse files Browse the repository at this point in the history
…t#9268)

## Describe your changes

This PR introduces new theme variables for `borderColor` (== fadedText10
) and `borderColorLight` (== fadedText05) and adds classes / fixes
test-ids for:

- [x] ChatMessage
- [x] Snow
- [x] Ballons
- [x] Audio
- [x] Video
- [x] Metric
- [x] ArrowTable
- [x] ExceptionElement
- [x] Expander
- [x] Toast
- [x] TextElement
- [x] Tabs
- [x] Spinner
- [x] Progress
- [x] Popover
- [x] PageLink
- [x] Markdown
- [x] LinkButton
- [x] HTML
- [x] BokehChart
- [x] Favicon
- [x] GraphVizChart
- [x] Iframe
- [x] Json

Every element should have a top-level class name and test-id that
follows our naming guidelines.

## Testing Plan

- Updated tests

---

**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
lukasmasuch authored Aug 15, 2024
1 parent ed96e3c commit 2625849
Show file tree
Hide file tree
Showing 122 changed files with 526 additions and 462 deletions.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
2 changes: 1 addition & 1 deletion e2e_playwright/st_balloons_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,4 +16,4 @@


def test_balloons_are_present_on_page(app: Page):
expect(app.get_by_test_id("balloons")).to_have_count(1)
expect(app.get_by_test_id("stBalloons")).to_have_count(1)
11 changes: 6 additions & 5 deletions e2e_playwright/st_expander_nested_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,13 +12,14 @@
# See the License for the specific language governing permissions and
# limitations under the License.

from playwright.sync_api import Page, expect
from playwright.sync_api import Page

from e2e_playwright.shared.app_utils import expect_exception


def test_nested_expanders(app: Page):
"""Test that st.expander may not be nested inside other expanders."""
exception_message = app.locator(".stException .message")

expect(exception_message).to_have_text(
"StreamlitAPIException: Expanders may not be nested inside other expanders."
expect_exception(
app,
"StreamlitAPIException: Expanders may not be nested inside other expanders.",
)
2 changes: 1 addition & 1 deletion e2e_playwright/st_snow_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,4 +16,4 @@


def test_snow_is_present_on_page(app: Page):
expect(app.get_by_test_id("snow")).to_have_count(1)
expect(app.get_by_test_id("stSnow")).to_have_count(1)
4 changes: 2 additions & 2 deletions frontend/app/src/components/AppView/styled-components.ts
Original file line number Diff line number Diff line change
Expand Up @@ -71,11 +71,12 @@ export const StyledAppViewMain = styled.section<StyledAppViewMainProps>(
})
)

export const StyledStickyBottomContainer = styled.div(() => ({
export const StyledStickyBottomContainer = styled.div(({ theme }) => ({
position: "sticky",
left: 0,
bottom: 0,
width: "100%",
zIndex: theme.zIndices.bottom,

// move the bottom container to the end of pages in print-mode so that nothing
// (e.g. a floating chat-input) overlays the actual app content
Expand All @@ -93,7 +94,6 @@ export const StyledInnerBottomContainer = styled.div(({ theme }) => ({
display: "flex",
flexDirection: "column",
alignItems: "center",
zIndex: theme.zIndices.bottom,
}))

export interface StyledAppViewBlockContainerProps {
Expand Down
4 changes: 2 additions & 2 deletions frontend/app/src/components/MainMenu/MainMenu.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -205,7 +205,7 @@ function buildMenuItemComponent(
}

const SubMenu = (props: SubMenuProps): ReactElement => {
const { colors }: EmotionTheme = useTheme()
const { colors, sizes }: EmotionTheme = useTheme()
const StyledMenuItemType = props.isDevMenu ? StyledDevItem : StyledCoreItem
return (
<StatefulMenu
Expand All @@ -231,7 +231,7 @@ const SubMenu = (props: SubMenuProps): ReactElement => {
":focus": {
outline: "none",
},
border: `1px solid ${colors.fadedText10}`,
border: `${sizes.borderWidth} solid ${colors.borderColor}`,
},
},
}}
Expand Down
4 changes: 2 additions & 2 deletions frontend/app/src/components/MainMenu/styled-components.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,9 @@
* limitations under the License.
*/

import styled from "@emotion/styled"
import { keyframes } from "@emotion/react"
import { Keyframes } from "@emotion/serialize"
import styled from "@emotion/styled"
import { transparentize } from "color2k"

import { EmotionTheme } from "@streamlit/lib"
Expand Down Expand Up @@ -45,7 +45,7 @@ export const StyledRecordingIndicator = styled.div(({ theme }) => ({
}))

export const StyledMenuDivider = styled.div(({ theme }) => ({
borderTop: `1px solid ${theme.colors.fadedText10}`,
borderTop: `${theme.sizes.borderWidth} solid ${theme.colors.borderColor}`,
margin: `${theme.spacing.sm} ${theme.spacing.none}`,
}))

Expand Down
6 changes: 3 additions & 3 deletions frontend/app/src/components/Sidebar/styled-components.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,14 +14,14 @@
* limitations under the License.
*/

import { transparentize } from "color2k"
import styled from "@emotion/styled"
import { transparentize } from "color2k"

import { StyledMaterialIcon } from "@streamlit/lib/src/components/shared/Icon/Material/styled-components"
import {
getWrappedHeadersStyle,
hasLightBackgroundColor,
} from "@streamlit/lib/src/theme/utils"
import { StyledMaterialIcon } from "@streamlit/lib/src/components/shared/Icon/Material/styled-components"

// Check for custom text color & handle colors in SidebarNav accordingly
const conditionalCustomColor = (
Expand Down Expand Up @@ -393,5 +393,5 @@ export const StyledViewButton = styled.button(({ theme }) => {

export const StyledSidebarNavSeparator = styled.div(({ theme }) => ({
paddingTop: theme.spacing.lg,
borderBottom: `1px solid ${theme.colors.fadedText10}`,
borderBottom: `${theme.sizes.borderWidth} solid ${theme.colors.borderColor}`,
}))
Original file line number Diff line number Diff line change
Expand Up @@ -28,32 +28,33 @@ interface IDeployCardProps {
function DeployCard(
props: React.PropsWithChildren<IDeployCardProps>
): ReactElement {
const { colors, spacing, radii, breakpoints }: EmotionTheme = useTheme()
const { colors, spacing, radii, breakpoints, sizes }: EmotionTheme =
useTheme()
const { children } = props
return (
<Card
overrides={{
Root: {
style: {
borderTopWidth: "1px",
borderBottomWidth: "1px",
borderLeftWidth: "1px",
borderRightWidth: "1px",
borderTopWidth: sizes.borderWidth,
borderBottomWidth: sizes.borderWidth,
borderLeftWidth: sizes.borderWidth,
borderRightWidth: sizes.borderWidth,

borderTopStyle: "solid",
borderBottomStyle: "solid",
borderLeftStyle: "solid",
borderRightStyle: "solid",

borderTopColor: colors.fadedText10,
borderBottomColor: colors.fadedText10,
borderLeftColor: colors.fadedText10,
borderRightColor: colors.fadedText10,
borderTopColor: colors.borderColor,
borderBottomColor: colors.borderColor,
borderLeftColor: colors.borderColor,
borderRightColor: colors.borderColor,

borderTopLeftRadius: radii.lg,
borderTopRightRadius: radii.lg,
borderBottomLeftRadius: radii.lg,
borderBottomRightRadius: radii.lg,
borderTopLeftRadius: radii.default,
borderTopRightRadius: radii.default,
borderBottomLeftRadius: radii.default,
borderBottomRightRadius: radii.default,
},
},
Contents: {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,8 @@
*/

import styled from "@emotion/styled"
import { ChevronLeft } from "react-feather"
import { darken } from "color2k"
import { ChevronLeft } from "react-feather"

import { Small } from "@streamlit/lib"

Expand Down Expand Up @@ -109,7 +109,7 @@ export const StyledButtonContainer = styled.div(({ theme }) => ({
export const StyledCheckbox = styled.input(({ theme }) => ({
marginRight: theme.spacing.xs,
appearance: "none",
border: `${theme.sizes.borderWidth} solid ${theme.colors.fadedText10}`,
border: `${theme.sizes.borderWidth} solid ${theme.colors.borderColor}`,
width: theme.fontSizes.md,
height: theme.fontSizes.md,
borderRadius: theme.radii.md,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ describe("ElementNodeRenderer Block Component", () => {
// eslint-disable-next-line testing-library/no-node-access
const elementRendererChildren = elementNodeRenderer.children
expect(elementRendererChildren).toHaveLength(1)
expect(elementRendererChildren[0]).toHaveClass("balloons")
expect(elementRendererChildren[0]).toHaveClass("stBalloons")
})
})

Expand Down Expand Up @@ -168,7 +168,7 @@ describe("ElementNodeRenderer Block Component", () => {
// eslint-disable-next-line testing-library/no-node-access
const elementRendererChildren = elementNodeRenderer.children
expect(elementRendererChildren).toHaveLength(1)
expect(elementRendererChildren[0]).toHaveClass("snow")
expect(elementRendererChildren[0]).toHaveClass("stSnow")
})
})
})
10 changes: 5 additions & 5 deletions frontend/lib/src/components/core/Block/styled-components.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,9 @@ import React from "react"

import styled from "@emotion/styled"

import { EmotionTheme } from "@streamlit/lib/src/theme"
import { StyledCheckbox } from "@streamlit/lib/src/components/widgets/Checkbox/styled-components"
import { Block as BlockProto } from "@streamlit/lib/src/proto"
import { EmotionTheme } from "@streamlit/lib/src/theme"

function translateGapWidth(gap: string, theme: EmotionTheme): string {
let gapWidth = theme.spacing.lg
Expand Down Expand Up @@ -73,19 +73,19 @@ export const StyledElementContainer = styled.div<StyledElementContainerProps>(
overflow: "visible",
},

":is(.empty-html)": {
":is(.stHtml-empty)": {
display: "none",
},

":has(> .cacheSpinner)": {
":has(> .stCacheSpinner)": {
height: 0,
overflow: "visible",
visibility: "visible",
marginBottom: "-1rem",
zIndex: 1000,
},

":has(> .stPageLink)": {
[`:has(> .stPageLink))`]: {
marginTop: "-0.375rem",
marginBottom: "-0.375rem",
},
Expand Down Expand Up @@ -197,7 +197,7 @@ export const StyledVerticalBlockBorderWrapper =
styled.div<StyledVerticalBlockBorderWrapperProps>(
({ theme, border, height }) => ({
...(border && {
border: `${theme.sizes.borderWidth} solid ${theme.colors.fadedText10}`,
border: `${theme.sizes.borderWidth} solid ${theme.colors.borderColor}`,
borderRadius: theme.radii.default,
padding: `calc(${theme.spacing.lg} - 1px)`, // 1px to account for border.
}),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ export default function AlertElement({
body,
kind,
width,
}: AlertElementProps): ReactElement {
}: Readonly<AlertElementProps>): ReactElement {
const markdownWidth = {
// Fix issue #6394 - Need to account for 1.25rem padding + 0.5rem gap when icon present
width: icon ? `calc(100% - 1.75rem)` : "100%",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,10 @@ describe("st._arrow_table", () => {
it("renders without crashing", () => {
const props = getProps(UNICODE)
render(<ArrowTable {...props} />)
const tableElement = screen.getByTestId("stTable")
expect(tableElement).toBeInTheDocument()
expect(tableElement).toHaveClass("stTable")

expect(screen.getByTestId("stTable")).toBeInTheDocument()
expect(screen.getByTestId("stTableStyledTable")).toBeInTheDocument()
expect(
screen.queryByTestId("stTableStyledEmptyTableCell")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ export interface TableProps {
element: Quiver
}

export function ArrowTable(props: TableProps): ReactElement {
export function ArrowTable(props: Readonly<TableProps>): ReactElement {
const table = props.element
const { cssId, cssStyles, caption } = table
const { headerRows, rows, columns } = table.dimensions
Expand All @@ -41,7 +41,7 @@ export function ArrowTable(props: TableProps): ReactElement {
const dataRows = allRows.slice(headerRows)

return (
<StyledTableContainer data-testid="stTable">
<StyledTableContainer className="stTable" data-testid="stTable">
{cssStyles && <style>{cssStyles}</style>}
<StyledTable id={cssId} data-testid="stTableStyledTable">
{caption && (
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,12 +30,12 @@ export const StyledTable = styled.table(({ theme }) => ({
marginBottom: theme.spacing.lg,
color: theme.colors.bodyText,
borderCollapse: "collapse",
border: `${theme.sizes.borderWidth} solid ${theme.colors.fadedText05}`,
border: `${theme.sizes.borderWidth} solid ${theme.colors.borderColorLight}`,
}))

const styleCellFunction = (theme: EmotionTheme): CSSObject => ({
borderBottom: `${theme.sizes.borderWidth} solid ${theme.colors.fadedText05}`,
borderRight: `${theme.sizes.borderWidth} solid ${theme.colors.fadedText05}`,
borderBottom: `${theme.sizes.borderWidth} solid ${theme.colors.borderColorLight}`,
borderRight: `${theme.sizes.borderWidth} solid ${theme.colors.borderColorLight}`,
verticalAlign: "middle",
padding: `${theme.spacing.twoXS} ${theme.spacing.xs}`,
fontWeight: theme.fontWeights.normal,
Expand Down
4 changes: 3 additions & 1 deletion frontend/lib/src/components/elements/Audio/Audio.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,9 @@ describe("Audio Element", () => {

it("renders without crashing", () => {
render(<Audio {...getProps()} />)
expect(screen.getByTestId("stAudio")).toBeInTheDocument()
const audioElement = screen.getByTestId("stAudio")
expect(audioElement).toBeInTheDocument()
expect(audioElement).toHaveClass("stAudio")
})

it("has controls", () => {
Expand Down
13 changes: 8 additions & 5 deletions frontend/lib/src/components/elements/Audio/Audio.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ export default function Audio({
width,
endpoints,
elementMgr,
}: AudioProps): ReactElement {
}: Readonly<AudioProps>): ReactElement {
const audioRef = useRef<HTMLAudioElement>(null)

const { startTime, endTime, loop, autoplay } = element
Expand Down Expand Up @@ -89,7 +89,9 @@ export default function Audio({
// Stop the audio at 'endTime' and handle loop
useEffect(() => {
const audioNode = audioRef.current
if (!audioNode) return
if (!audioNode) {
return
}

// Flag to avoid calling 'audioNode.pause()' multiple times
let stoppedByEndTime = false
Expand Down Expand Up @@ -121,7 +123,9 @@ export default function Audio({
// Handle looping the audio
useEffect(() => {
const audioNode = audioRef.current
if (!audioNode) return
if (!audioNode) {
return
}

// Loop the audio when it has ended
const handleAudioEnd = (): void => {
Expand All @@ -144,13 +148,12 @@ export default function Audio({

return (
<audio
className="stAudio"
data-testid="stAudio"
id="audio"
ref={audioRef}
controls
autoPlay={autoplay && !preventAutoplay}
src={uri}
className="stAudio"
style={{ width }}
/>
)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,8 +39,9 @@ describe("Balloons element", () => {
const props = getProps()
render(<Balloons {...props} />)

const balloonElement = screen.getByTestId("balloons")
const balloonElement = screen.getByTestId("stBalloons")
expect(balloonElement).toBeInTheDocument()
expect(balloonElement).toHaveClass("stBalloons")

const balloonImages = screen.getAllByRole("img")
expect(balloonImages.length).toBe(NUM_BALLOONS)
Expand All @@ -54,7 +55,7 @@ describe("Balloons element", () => {
const props = getProps()
render(<Balloons {...props} />)

const balloonElement = screen.getByTestId("balloons")
const balloonElement = screen.getByTestId("stBalloons")
expect(balloonElement).toHaveClass("stHidden")
})
})
3 changes: 2 additions & 1 deletion frontend/lib/src/components/elements/Balloons/Balloons.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,8 @@ const Balloons: FC<React.PropsWithChildren<Props>> = ({ scriptRunId }) => (
// Keys should be unique each time, so React replaces the images in the DOM and their animations
// actually rerun.
<Particles
className="balloons"
className="stBalloons"
data-testid="stBalloons"
scriptRunId={scriptRunId}
numParticleTypes={NUM_BALLOON_TYPES}
numParticles={NUM_BALLOONS}
Expand Down
Loading

0 comments on commit 2625849

Please sign in to comment.