Skip to content

Commit

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

## Describe your changes

Applies a couple of small styling refactorings and adds/unifies
classnames & data-testid's for:

- [x] AlertElement
- [x] CodeBlock
- [x] Dialog
- [x] DocString
- [x] ImageList
- [x] Particles
- [x] PlotlyChart
- [x] shared/Modal
- [x] shared/AlertElement
- [x] shared/ErrorBoundary
- [x] shared/ErrorElement
- [x] shared/Icon
- [x] shared/InputInstructions
- [x] shared/TextElements
- [x] shared/Tooltip
- [x] shared/TooltipIcon

## Testing Plan

- Not logical changes -> no tests required. 

---

**Contribution License Agreement**

By submitting this pull request you agree that all contributions to this
project are made under the Apache 2.0 license.

---------

Co-authored-by: Lukas Masuch <[email protected]>
  • Loading branch information
lukasmasuch and sfc-gh-lmasuch authored Aug 19, 2024
1 parent 29440d7 commit 2910247
Show file tree
Hide file tree
Showing 55 changed files with 183 additions and 164 deletions.
4 changes: 2 additions & 2 deletions e2e/specs/st_image.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ describe("st.image", () => {
});

it("displays the image and caption together", () => {
cy.get(".element-container [data-testid='stImage']")
cy.get(".element-container [data-testid='stImageContainer']")
.eq(0)
.matchImageSnapshot("image-with-caption");
});
Expand Down Expand Up @@ -103,7 +103,7 @@ describe("st.image", () => {

it("displays a GIF image and a caption together", () => {
cy.getIndexed(
".element-container [data-testid='stImage']",
".element-container [data-testid='stImageContainer']",
14
).matchImageSnapshot("gif-with-caption");
});
Expand Down
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.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
2 changes: 1 addition & 1 deletion e2e_playwright/deploy_dialog_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ def test_deploy_button_displays_correctly(

# Make sure that deploy dialog is properly displayed
# Before taking screenshot
deploy_dialog = themed_app.get_by_test_id("stModal")
deploy_dialog = themed_app.get_by_test_id("stDialog")
expect(deploy_dialog).to_be_visible()
expect(
deploy_dialog.get_by_test_id("stDeployDialogCommunityCloudIcon")
Expand Down
12 changes: 6 additions & 6 deletions e2e_playwright/main_menu_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ def test_renders_settings_dialog_properly(
themed_app.get_by_test_id("stMainMenu").click()

themed_app.get_by_text("Settings").click()
dialog = themed_app.get_by_test_id("stModal")
dialog = themed_app.get_by_test_id("stDialog")
expect(dialog).to_be_visible()
assert_snapshot(dialog.get_by_role("dialog"), name="settings_dialog")

Expand All @@ -44,7 +44,7 @@ def test_renders_screencast_dialog_properly(
themed_app.get_by_test_id("stMainMenu").click()

themed_app.get_by_text("Record a screencast").click()
dialog = themed_app.get_by_test_id("stModal")
dialog = themed_app.get_by_test_id("stDialog")
expect(dialog).to_be_visible()
assert_snapshot(dialog.get_by_role("dialog"), name="record_screencast_dialog")

Expand All @@ -62,7 +62,7 @@ def test_renders_screencast_recorded_dialog_properly(themed_app: Page):

# stop recording
themed_app.keyboard.press("Escape")
dialog = themed_app.get_by_test_id("stModal")
dialog = themed_app.get_by_test_id("stDialog")
expect(dialog).to_be_visible()

# don't use screenshot as the recording may differ so just check for specific text
Expand All @@ -75,7 +75,7 @@ def test_renders_about_dialog_properly(themed_app: Page):
themed_app.get_by_test_id("stMainMenu").click()

themed_app.get_by_text("About").click()
dialog = themed_app.get_by_test_id("stModal")
dialog = themed_app.get_by_test_id("stDialog")
expect(dialog).to_be_visible()
expect(dialog).to_contain_text("Made with Streamlit v")

Expand All @@ -86,7 +86,7 @@ def test_renders_clear_cache_dialog_properly(
themed_app.get_by_test_id("stMainMenu").click()

themed_app.get_by_text("Clear cache").click()
dialog = themed_app.get_by_test_id("stModal")
dialog = themed_app.get_by_test_id("stDialog")
expect(dialog).to_be_visible()
expect(dialog).to_contain_text(
"Are you sure you want to clear the app's function caches?"
Expand All @@ -101,7 +101,7 @@ def test_renders_active_theme_dialog_properly(
themed_app.get_by_text("Settings").click()
themed_app.get_by_text("Edit active theme").click()

dialog = themed_app.get_by_test_id("stModal")
dialog = themed_app.get_by_test_id("stDialog")
expect(dialog).to_be_visible()

assert_snapshot(dialog.get_by_role("dialog"), name="edit_active_theme_dialog")
10 changes: 5 additions & 5 deletions e2e_playwright/st_code_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,22 +19,22 @@

def test_code_display(app: Page):
"""Test that st.code displays a code block."""
code_element = app.locator(".element-container pre").first
code_element = app.get_by_test_id("stCode").first
expect(code_element).to_contain_text("This code is awesome!")


def test_syntax_highlighting(themed_app: Page, assert_snapshot: ImageCompareFunction):
"""Test that the copy-to-clipboard action appears on hover."""
first_code_element = themed_app.locator(".element-container:first-child pre").first
first_code_element = themed_app.get_by_test_id("stCode").first
first_code_element.hover()
assert_snapshot(first_code_element, name="syntax_highlighting-hover")
assert_snapshot(first_code_element, name="st_code-hover_copy")


def test_code_blocks_render_correctly(
themed_app: Page, assert_snapshot: ImageCompareFunction
):
"""Test that the code blocks render as expected via screenshot matching."""
code_blocks = themed_app.get_by_test_id("stCodeBlock")
code_blocks = themed_app.get_by_test_id("stCode")
expect(code_blocks).to_have_count(11)

assert_snapshot(code_blocks.nth(0), name="st_code-auto_lang")
Expand All @@ -51,7 +51,7 @@ def test_correct_bottom_spacing_for_code_blocks(app: Page):

# The first code block should have no bottom margin:
expect(
app.get_by_test_id("stExpander").nth(0).get_by_test_id("stCodeBlock").first
app.get_by_test_id("stExpander").nth(0).get_by_test_id("stCode").first
).to_have_css("margin-bottom", "0px")
# While the codeblock used inside markdown should have a bottom margin to imitate the gap:
expect(
Expand Down
2 changes: 1 addition & 1 deletion e2e_playwright/st_dialog_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
from e2e_playwright.conftest import ImageCompareFunction, wait_for_app_run
from e2e_playwright.shared.app_utils import COMMAND_KEY, get_markdown

modal_test_id = "stModal"
modal_test_id = "stDialog"


def open_dialog_with_images(app: Page):
Expand Down
2 changes: 1 addition & 1 deletion e2e_playwright/st_help_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@

def test_help_display(app: Page, assert_snapshot: ImageCompareFunction):
"""Test that st.header renders correctly with dividers."""
help_elements = app.get_by_test_id("stDocstring")
help_elements = app.get_by_test_id("stHelp")

for i, element in enumerate(help_elements.all()):
assert_snapshot(element, name=f"st_help-{i}")
5 changes: 3 additions & 2 deletions e2e_playwright/st_pyplot_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -24,14 +24,15 @@ def test_displays_a_pyplot_figures(
themed_app: Page, assert_snapshot: ImageCompareFunction
):
"""Test that all pyplot figures are displayed correctly via screenshot matching."""
pyplot_elements = themed_app.get_by_test_id("stImage")
expect(pyplot_elements).to_have_count(8)

# pyplot graph assertion
expect(themed_app.get_by_test_id("stImage").last.locator("img")).to_have_attribute(
"src", re.compile("localhost*")
)

pyplot_elements = themed_app.get_by_test_id("stImage").locator("img")
expect(pyplot_elements).to_have_count(8)

assert_snapshot(pyplot_elements.nth(0), name="st_pyplot-normal_figure")
assert_snapshot(pyplot_elements.nth(1), name="st_pyplot-resized_figure")
assert_snapshot(pyplot_elements.nth(2), name="st_pyplot-container_width_true")
Expand Down
2 changes: 1 addition & 1 deletion e2e_playwright/st_write_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ def test_display_json(app: Page):

def test_display_help(app: Page):
"""Test that st.write displays objects via st.help."""
help_elements = app.get_by_test_id("stDocstring")
help_elements = app.get_by_test_id("stHelp")
expect(help_elements).to_have_count(3)


Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ describe("aboutDialog", () => {
</Fragment>
)

expect(screen.getByTestId("stModal")).toBeInTheDocument()
expect(screen.getByTestId("stDialog")).toBeInTheDocument()
// need a regex because there is a line break
const versionRegex = /Streamlit v\s*42\.42\.42/
const versionText = screen.getByText(versionRegex)
Expand All @@ -119,7 +119,7 @@ describe("aboutDialog", () => {
</Fragment>
)

expect(screen.getByTestId("stModal")).toBeInTheDocument()
expect(screen.getByTestId("stDialog")).toBeInTheDocument()
// regex that is anything after Streamlit v
const versionRegex = /^Streamlit v.*/
const nonExistentText = screen.queryByText(versionRegex)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ describe("UnsupportedBrowserDialog", () => {
</BaseProvider>
)

expect(screen.getByTestId("stModal")).toBeInTheDocument()
expect(screen.getByTestId("stDialog")).toBeInTheDocument()
expect(
screen.getByTestId("stUnsupportedBrowserDialog")
).toBeInTheDocument()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ describe("VideoRecordedDialog", () => {
<VideoRecordedDialog {...props} />
</BaseProvider>
)
expect(screen.getByTestId("stModal")).toBeInTheDocument()
expect(screen.getByTestId("stDialog")).toBeInTheDocument()
expect(screen.getByTestId("stVideoRecordedDialog")).toBeInTheDocument()
})

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,10 +44,11 @@ describe("Alert element", () => {
body: "#what in the world?",
})
render(<AlertElement {...props} />)
expect(screen.getByTestId("stAlert")).toBeInTheDocument()
expect(
screen.getByTestId("stNotificationContentError")
).toBeInTheDocument()
const alertElement = screen.getByTestId("stAlert")
expect(alertElement).toBeInTheDocument()
expect(alertElement).toHaveClass("stAlert")

expect(screen.getByTestId("stAlertContentError")).toBeInTheDocument()
expect(screen.queryByTestId("stAlertDynamicIcon")).not.toBeInTheDocument()
expect(screen.getByText("#what in the world?")).toBeInTheDocument()
})
Expand All @@ -59,9 +60,7 @@ describe("Alert element", () => {
})
render(<AlertElement {...props} />)
expect(screen.getByTestId("stAlert")).toBeInTheDocument()
expect(
screen.getByTestId("stNotificationContentWarning")
).toBeInTheDocument()
expect(screen.getByTestId("stAlertContentWarning")).toBeInTheDocument()
expect(screen.queryByTestId("stAlertDynamicIcon")).not.toBeInTheDocument()
expect(screen.getByText("test")).toBeInTheDocument()
})
Expand All @@ -73,9 +72,7 @@ describe("Alert element", () => {
})
render(<AlertElement {...props} />)
expect(screen.getByTestId("stAlert")).toBeInTheDocument()
expect(
screen.getByTestId("stNotificationContentSuccess")
).toBeInTheDocument()
expect(screen.getByTestId("stAlertContentSuccess")).toBeInTheDocument()
expect(screen.queryByTestId("stAlertDynamicIcon")).not.toBeInTheDocument()
expect(
screen.getByText("But our princess was in another castle!")
Expand All @@ -89,7 +86,7 @@ describe("Alert element", () => {
})
render(<AlertElement {...props} />)
expect(screen.getByTestId("stAlert")).toBeInTheDocument()
expect(screen.getByTestId("stNotificationContentInfo")).toBeInTheDocument()
expect(screen.getByTestId("stAlertContentInfo")).toBeInTheDocument()
expect(screen.queryByTestId("stAlertDynamicIcon")).not.toBeInTheDocument()
expect(screen.getByText("It's dangerous to go alone.")).toBeInTheDocument()
})
Expand All @@ -102,7 +99,7 @@ describe("Alert element", () => {
})
render(<AlertElement {...props} />)
expect(screen.getByTestId("stAlert")).toBeInTheDocument()
expect(screen.getByTestId("stNotificationContentInfo")).toBeInTheDocument()
expect(screen.getByTestId("stAlertContentInfo")).toBeInTheDocument()
expect(screen.getByTestId("stAlertDynamicIcon")).toHaveTextContent("👉🏻")
expect(screen.getByText("It's dangerous to go alone.")).toBeInTheDocument()
})
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,29 +20,20 @@ import {
StyledEmojiIcon,
StyledIcon,
} from "@streamlit/lib/src/components/shared/Icon/styled-components"
import { StyledCodeBlock } from "@streamlit/lib/src/components/elements/CodeBlock/styled-components"
import { StyledMaterialIcon } from "@streamlit/lib/src/components/shared/Icon/Material/styled-components"

export const StyledAlertContent = styled.div(({ theme }) => ({
display: "flex",
gap: theme.spacing.sm,
width: "100%",

[StyledEmojiIcon as any]: {
[`${StyledEmojiIcon}, ${StyledIcon}, ${StyledMaterialIcon}`]: {
position: "relative",
top: "2px",
},

[StyledIcon as any]: {
position: "relative",
top: "2px",
},

[StyledMaterialIcon as any]: {
position: "relative",
top: "2px",
},

".stCodeBlock code": {
paddingRight: "1rem",
[`${StyledCodeBlock} code`]: {
paddingRight: theme.spacing.lg,
},
}))
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ export default function StreamlitSyntaxHighlighter({
children,
}: Readonly<StreamlitSyntaxHighlighterProps>): ReactElement {
return (
<StyledCodeBlock className="stCodeBlock" data-testid="stCodeBlock">
<StyledCodeBlock className="stCode" data-testid="stCode">
<StyledPre>
<SyntaxHighlighter
language={language}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -152,13 +152,12 @@ export const StyledCodeBlock = styled.div(({ theme }) => ({

export const StyledCopyButton = styled.button(({ theme }) => ({
pointerEvents: "auto",
height: "2.5rem",
padding: 0,
width: "2.5rem",
height: theme.iconSizes.threeXL,
width: theme.iconSizes.threeXL,
padding: theme.spacing.none,
border: "none",
backgroundColor: theme.colors.transparent,
color: theme.colors.fadedText60,
borderRadius: theme.radii.xl,
transform: "scale(0)",

[`${StyledCodeBlock}:hover &, &:active, &:focus, &:hover`]: {
Expand Down
3 changes: 2 additions & 1 deletion frontend/lib/src/components/elements/Dialog/Dialog.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -46,8 +46,9 @@ describe("Dialog container", () => {
</Dialog>
)

const dialogContainer = screen.getByTestId("stModal")
const dialogContainer = screen.getByTestId("stDialog")
expect(dialogContainer).toBeInTheDocument()
expect(dialogContainer).toHaveClass("stDialog")
})

it("should render the text when open", () => {
Expand Down
Loading

0 comments on commit 2910247

Please sign in to comment.