Skip to content

Commit

Permalink
Refactor frontend styles, class names and test-ids - Part 3 (#9315)
Browse files Browse the repository at this point in the history
## Describe your changes

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

- [x] widgets/TimeInput
- [x] widgets/TextInput
- [x] widgets/TextArea
- [x] widget/Slider
- [x] widgets/Selectbox
- [x] widgets/Radio
- [x] widgets/NumberInput
- [x] widgets/Multiselect
- [x] widgets/Form
- [x] widgets/FileUploader
- [x] widgets/DownloadButton
- [x] widgets/DateInput
- [x] widgets/DataFrame
- [x] widgets/CustomComponent
- [x] widgets/ColorPicker
- [x] widgets/Checkbox
- [x] widgets/ChatInput
- [x] widgets/CameraInput
- [x] widgets/ButtonGroup
- [x] widgets/Button
- [x] widgets/BaseWidget
- [x] shared/Toolbar
- [x] shared/StreamlitMarkdown
- [x] shared/Radio
- [x] shared/ProgressBar
- [x] ArrowVegaLiteChart
- [x] DeckGlJsonChart
- [x] shared/BaseButton

With this project, I'm going through all react components in multiple
chunks to:

1. Replace hard-coded styling attributes with theming variables (focused
on simple, non-risky changes) -> This is an initial step for the
advanced theming project.
2. Add stable class names to the root of all standalone elements to
provide slightly more stability and simplicity for CSS hacks. Class
names should follow the naming structure for consistency and to ensure
uniqueness of class names. This also includes a couple of changes to
existing class names, but we will keep old, non-aligned class names
around in addition to the new class names if there is an indication of
significant usage for existing CSS hacks.
3. Clean up test IDs to follow the same naming structure (most already
do). This isn't too important; consistency is nice and hopefully will
encourage others to follow the pattern. And it might prevent some
potential overlaps in test-id naming.

Naming structure for class names & test-ids: class names should be camel
case, prefixed with `st`, and contain a descriptive term that refers to
the element (e.g. the command name).

In addition to these goals, the PR also cleans up our e2e tests to use
test-ids wherever possible and to use utility methods in `app_utils` for
some interactions.

## Testing Plan

- No logical changes -> no tests required.
- Added e2e tests to all elements to check that it has a top-level
className.

---

**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 22, 2024
1 parent 42af833 commit e91bea8
Show file tree
Hide file tree
Showing 177 changed files with 919 additions and 495 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.
2 changes: 1 addition & 1 deletion e2e_playwright/deploy_dialog_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
def test_deploy_button_displays_correctly(
themed_app: Page, assert_snapshot: ImageCompareFunction
):
deploy_button = themed_app.get_by_test_id("stDeployButton")
deploy_button = themed_app.get_by_test_id("stAppDeployButton")
deploy_button.click()

# Make sure that deploy dialog is properly displayed
Expand Down
16 changes: 8 additions & 8 deletions e2e_playwright/hello_app_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -70,9 +70,9 @@ def test_plotting_demo_page(app: Page, assert_snapshot: ImageCompareFunction) ->
# and additional timeout
expect(app.get_by_test_id("stText")).to_contain_text("100% Complete", timeout=15000)
expect(app.get_by_test_id("stProgress")).not_to_be_visible()
expect(
app.get_by_test_id("stArrowVegaLiteChart").locator("canvas")
).to_have_attribute("height", "350")
expect(app.get_by_test_id("stVegaLiteChart").locator("canvas")).to_have_attribute(
"height", "350"
)

assert_snapshot(app, name="hello_app-plotting_demo_page")

Expand Down Expand Up @@ -101,9 +101,9 @@ def _load_dataframe_demo_page(app: Page):
check_page_icon(app, "table", 4)
expect(app.get_by_test_id("stMultiSelect")).to_be_visible()
expect(app.get_by_test_id("stDataFrame")).to_be_visible()
expect(
app.get_by_test_id("stArrowVegaLiteChart").locator("canvas")
).to_have_attribute("height", "350")
expect(app.get_by_test_id("stVegaLiteChart").locator("canvas")).to_have_attribute(
"height", "350"
)


def test_dataframe_demo_page(app: Page, assert_snapshot: ImageCompareFunction) -> None:
Expand Down Expand Up @@ -161,7 +161,7 @@ def test_app_print_mode_portrait_with_sidebar_closed(
# close sidebar. Must be done before print-mode, because we hide the close button when printing
app.get_by_test_id("stSidebar").hover()
sidebar_element = app.get_by_test_id("stSidebarContent")
sidebar_element.get_by_test_id("baseButton-headerNoPadding").click()
sidebar_element.get_by_test_id("stBaseButton-headerNoPadding").click()
expect(sidebar_element).not_to_be_visible()

app.emulate_media(media="print", forced_colors="active")
Expand Down Expand Up @@ -196,7 +196,7 @@ def test_app_print_mode_landscape_with_sidebar_closed(
# close sidebar. Must be done before print-mode, because we hide the close button when printing
app.get_by_test_id("stSidebar").hover()
sidebar_element = app.get_by_test_id("stSidebarContent")
sidebar_element.get_by_test_id("baseButton-headerNoPadding").click()
sidebar_element.get_by_test_id("stBaseButton-headerNoPadding").click()
expect(sidebar_element).not_to_be_visible()

app.emulate_media(media="print", forced_colors="active")
Expand Down
7 changes: 3 additions & 4 deletions e2e_playwright/multipage_apps/mpa_basics_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
wait_for_app_loaded,
wait_for_app_run,
)
from e2e_playwright.shared.app_utils import click_button


def test_loads_main_script_on_initial_page_load(app: Page):
Expand Down Expand Up @@ -147,13 +148,11 @@ def test_switch_page(app: Page):
expect(app.get_by_test_id("stHeading")).to_contain_text("Page 2")

# st.switch_page using relative path & leading /
app.get_by_test_id("baseButton-secondary").click()
wait_for_app_run(app)
click_button(app, "pages/06_page_6.py")
expect(app.get_by_test_id("stHeading")).to_contain_text("Page 6")

# st.switch_page using relative path & leading ./
app.get_by_test_id("baseButton-secondary").click()
wait_for_app_run(app)
click_button(app, "./mpa_basics.py")
expect(app.get_by_test_id("stHeading")).to_contain_text("Main Page")


Expand Down
12 changes: 5 additions & 7 deletions e2e_playwright/multipage_apps_v2/mpa_v2_basics_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
wait_for_app_loaded,
wait_for_app_run,
)
from e2e_playwright.shared.app_utils import click_checkbox
from e2e_playwright.shared.app_utils import click_button, click_checkbox


def main_heading(app: Page):
Expand Down Expand Up @@ -209,17 +209,15 @@ def test_handles_expand_collapse_of_mpa_nav_correctly(
def test_switch_page_by_path(app: Page):
"""Test that we can switch between pages by triggering st.switch_page with a path."""

app.get_by_test_id("baseButton-secondary").filter(has_text="page 5").click()
wait_for_app_run(app)
click_button(app, "page 5")

expect(page_heading(app)).to_contain_text("Page 5")


def test_switch_page_by_st_page(app: Page):
"""Test that we can switch between pages by triggering st.switch_page with st.Page."""

app.get_by_test_id("baseButton-secondary").filter(has_text="page 9").click()
wait_for_app_run(app)
click_button(app, "page 9")

expect(page_heading(app)).to_contain_text("Page 9")

Expand All @@ -233,8 +231,8 @@ def test_removes_query_params_with_st_switch_page(app: Page, app_port: int):
expect(app).to_have_url(f"http://localhost:{app_port}/?foo=bar")

# Trigger st.switch_page
app.get_by_test_id("baseButton-secondary").filter(has_text="page 5").click()
wait_for_app_loaded(app)
click_button(app, "page 5")

# Check that query params don't persist
expect(app).to_have_url(f"http://localhost:{app_port}/page_5")

Expand Down
23 changes: 21 additions & 2 deletions e2e_playwright/shared/app_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -116,8 +116,10 @@ def get_form_submit_button(
Locator
The element.
"""
element = locator.get_by_test_id("baseButton-secondaryFormSubmit").filter(
has_text=label
element = (
locator.get_by_test_id("stFormSubmitButton")
.filter(has_text=label)
.locator("button")
)
expect(element).to_be_visible()
return element
Expand Down Expand Up @@ -461,3 +463,20 @@ def expand_sidebar(app: Page) -> Locator:
sidebar = app.get_by_test_id("stSidebar")
expect(sidebar).to_be_visible()
return sidebar


def check_top_level_class(app: Page, test_id: str) -> None:
"""Check that the top level class is correctly set.
It should be the same as the test id of the element
and set on the same component.
Parameters
----------
app : Page
The page to search for the element.
test_id : str
The test id of the element to check.
"""
expect(app.get_by_test_id(test_id).first).to_have_class(re.compile(test_id))
4 changes: 2 additions & 2 deletions e2e_playwright/st_add_rows_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
def test_that_no_new_elements_are_created(themed_app: Page):
expect(themed_app.get_by_test_id("stTable")).to_have_count(1)
expect(themed_app.get_by_test_id("stDataFrame")).to_have_count(1)
expect(themed_app.get_by_test_id("stArrowVegaLiteChart")).to_have_count(7)
expect(themed_app.get_by_test_id("stVegaLiteChart")).to_have_count(7)


def test_correctly_adds_rows_to_table(themed_app: Page):
Expand All @@ -30,7 +30,7 @@ def test_correctly_adds_rows_to_table(themed_app: Page):
def test_correctly_adds_rows_to_charts(
themed_app: Page, assert_snapshot: ImageCompareFunction
):
charts = themed_app.get_by_test_id("stArrowVegaLiteChart")
charts = themed_app.get_by_test_id("stVegaLiteChart")
for index in range(charts.count()):
assert_snapshot(
charts.nth(index), name=f"st_vega_lite_chart-added_rows-{index}"
Expand Down
6 changes: 6 additions & 0 deletions e2e_playwright/st_alert_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
from playwright.sync_api import Page, expect

from e2e_playwright.conftest import ImageCompareFunction
from e2e_playwright.shared.app_utils import check_top_level_class


def test_alerts_rendering(themed_app: Page, assert_snapshot: ImageCompareFunction):
Expand Down Expand Up @@ -48,3 +49,8 @@ def test_alerts_rendering(themed_app: Page, assert_snapshot: ImageCompareFunctio
assert_snapshot(alert_elements.nth(17), name="st_alert-warning_non_emoji_icon")
assert_snapshot(alert_elements.nth(18), name="st_alert-info_non_emoji_icon")
assert_snapshot(alert_elements.nth(19), name="st_alert-success_non_emoji_icon")


def test_check_top_level_class(app: Page):
"""Check that the top level class is correctly set."""
check_top_level_class(app, "stAlert")
27 changes: 13 additions & 14 deletions e2e_playwright/st_altair_chart_basic_select_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
from playwright.sync_api import Locator, Page, expect

from e2e_playwright.conftest import ImageCompareFunction, wait_for_app_run
from e2e_playwright.shared.app_utils import expect_prefixed_markdown
from e2e_playwright.shared.app_utils import click_form_button, expect_prefixed_markdown


@dataclass
Expand Down Expand Up @@ -62,47 +62,47 @@ def _click(app: Page, chart: Locator, click_position: _MousePosition) -> None:


def _get_selection_point_scatter_chart(app: Page) -> Locator:
return app.get_by_test_id("stArrowVegaLiteChart").locator("canvas").nth(0)
return app.get_by_test_id("stVegaLiteChart").locator("canvas").nth(0)


def _get_selection_interval_scatter_chart(app: Page) -> Locator:
return app.get_by_test_id("stArrowVegaLiteChart").locator("canvas").nth(1)
return app.get_by_test_id("stVegaLiteChart").locator("canvas").nth(1)


def _get_selection_point_bar_chart(app: Page) -> Locator:
return app.get_by_test_id("stArrowVegaLiteChart").locator("canvas").nth(2)
return app.get_by_test_id("stVegaLiteChart").locator("canvas").nth(2)


def _get_selection_interval_bar_chart(app: Page) -> Locator:
return app.get_by_test_id("stArrowVegaLiteChart").locator("canvas").nth(3)
return app.get_by_test_id("stVegaLiteChart").locator("canvas").nth(3)


def _get_selection_point_area_chart(app: Page) -> Locator:
return app.get_by_test_id("stArrowVegaLiteChart").locator("canvas").nth(4)
return app.get_by_test_id("stVegaLiteChart").locator("canvas").nth(4)


def _get_selection_interval_area_chart(app: Page) -> Locator:
return app.get_by_test_id("stArrowVegaLiteChart").locator("canvas").nth(5)
return app.get_by_test_id("stVegaLiteChart").locator("canvas").nth(5)


def _get_selection_point_histogram(app: Page) -> Locator:
return app.get_by_test_id("stArrowVegaLiteChart").locator("canvas").nth(6)
return app.get_by_test_id("stVegaLiteChart").locator("canvas").nth(6)


def _get_selection_interval_histogram(app: Page) -> Locator:
return app.get_by_test_id("stArrowVegaLiteChart").locator("canvas").nth(7)
return app.get_by_test_id("stVegaLiteChart").locator("canvas").nth(7)


def _get_in_form_chart(app: Page) -> Locator:
return app.get_by_test_id("stArrowVegaLiteChart").locator("canvas").nth(8)
return app.get_by_test_id("stVegaLiteChart").locator("canvas").nth(8)


def _get_callback_chart(app: Page) -> Locator:
return app.get_by_test_id("stArrowVegaLiteChart").locator("canvas").nth(9)
return app.get_by_test_id("stVegaLiteChart").locator("canvas").nth(9)


def _get_in_fragment_chart(app: Page) -> Locator:
return app.get_by_test_id("stArrowVegaLiteChart").locator("canvas").nth(10)
return app.get_by_test_id("stVegaLiteChart").locator("canvas").nth(10)


def test_point_bar_chart_displays_selection_text(app: Page):
Expand Down Expand Up @@ -286,8 +286,7 @@ def test_in_form_selection_and_session_state(app: Page):

# submit the form. The selection uses a debounce of 200ms; if we click too early, the state is not updated correctly and we submit the old, unselected values
# app.wait_for_timeout(210)
app.get_by_test_id("baseButton-secondaryFormSubmit").click()
wait_for_app_run(app)
click_form_button(app, "Submit")

expected_selection = re.compile(
"{'selection': {'param_1': \\[{'IMDB_Rating': 4.6}\\]}}"
Expand Down
10 changes: 8 additions & 2 deletions e2e_playwright/st_altair_chart_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,15 +15,16 @@
from playwright.sync_api import Page, expect

from e2e_playwright.conftest import ImageCompareFunction
from e2e_playwright.shared.app_utils import check_top_level_class


def test_altair_chart_displays_correctly(
themed_app: Page, assert_snapshot: ImageCompareFunction
):
expect(
themed_app.get_by_test_id("stArrowVegaLiteChart").locator("canvas")
themed_app.get_by_test_id("stVegaLiteChart").locator("canvas")
).to_have_count(10)
charts = themed_app.get_by_test_id("stArrowVegaLiteChart")
charts = themed_app.get_by_test_id("stVegaLiteChart")
expect(charts).to_have_count(10)
snapshot_names = [
"st_altair_chart-scatter_chart_default_theme",
Expand All @@ -42,3 +43,8 @@ def test_altair_chart_displays_correctly(
# We should probably remove this once we have refactored the
# altair frontend component.
assert_snapshot(charts.nth(i), name=name, image_threshold=0.6)


def test_check_top_level_class(app: Page):
"""Check that the top level class is correctly set."""
check_top_level_class(app, "stVegaLiteChart")
10 changes: 8 additions & 2 deletions e2e_playwright/st_area_chart_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,13 +15,14 @@
from playwright.sync_api import Page, expect

from e2e_playwright.conftest import ImageCompareFunction
from e2e_playwright.shared.app_utils import check_top_level_class

TOTAL_AREA_CHARTS = 15


def test_area_chart_rendering(app: Page, assert_snapshot: ImageCompareFunction):
"""Test that st.area_chart renders correctly via snapshot testing."""
area_chart_elements = app.get_by_test_id("stArrowVegaLiteChart")
area_chart_elements = app.get_by_test_id("stVegaLiteChart")
expect(area_chart_elements).to_have_count(TOTAL_AREA_CHARTS)

# Also make sure that all canvas objects are rendered:
Expand All @@ -35,11 +36,16 @@ def test_themed_area_chart_rendering(
themed_app: Page, assert_snapshot: ImageCompareFunction
):
"""Test that st.area_chart renders with different theming."""
area_chart_elements = themed_app.get_by_test_id("stArrowVegaLiteChart")
area_chart_elements = themed_app.get_by_test_id("stVegaLiteChart")
expect(area_chart_elements).to_have_count(TOTAL_AREA_CHARTS)

# Also make sure that all canvas objects are rendered:
expect(area_chart_elements.locator("canvas")).to_have_count(TOTAL_AREA_CHARTS)

# Only test a single chart per built-in chart type:
assert_snapshot(area_chart_elements.nth(1), name="st_area_chart_themed")


def test_check_top_level_class(app: Page):
"""Check that the top level class is correctly set."""
check_top_level_class(app, "stVegaLiteChart")
11 changes: 10 additions & 1 deletion e2e_playwright/st_audio_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,11 @@
from playwright.sync_api import Page, expect

from e2e_playwright.conftest import wait_until
from e2e_playwright.shared.app_utils import click_button, click_checkbox
from e2e_playwright.shared.app_utils import (
check_top_level_class,
click_button,
click_checkbox,
)


def test_audio_has_correct_properties(app: Page):
Expand Down Expand Up @@ -106,3 +110,8 @@ def test_audio_remount_no_autoplay(app: Page):

expect(audio_element).to_have_js_property("autoplay", False)
expect(audio_element).to_have_js_property("paused", True)


def test_check_top_level_class(app: Page):
"""Check that the top level class is correctly set."""
check_top_level_class(app, "stAudio")
3 changes: 3 additions & 0 deletions e2e_playwright/st_balloons_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,9 @@

from playwright.sync_api import Page, expect

from e2e_playwright.shared.app_utils import check_top_level_class


def test_balloons_are_present_on_page(app: Page):
expect(app.get_by_test_id("stBalloons")).to_have_count(1)
check_top_level_class(app, "stBalloons")
10 changes: 8 additions & 2 deletions e2e_playwright/st_bar_chart_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,13 +15,14 @@
from playwright.sync_api import Page, expect

from e2e_playwright.conftest import ImageCompareFunction
from e2e_playwright.shared.app_utils import check_top_level_class

TOTAL_BAR_CHARTS = 18


def test_bar_chart_rendering(app: Page, assert_snapshot: ImageCompareFunction):
"""Test that st.bar_chart renders correctly via snapshot testing."""
bar_chart_elements = app.get_by_test_id("stArrowVegaLiteChart")
bar_chart_elements = app.get_by_test_id("stVegaLiteChart")
expect(bar_chart_elements).to_have_count(TOTAL_BAR_CHARTS)

# Also make sure that all canvas objects are rendered:
Expand All @@ -35,11 +36,16 @@ def test_themed_bar_chart_rendering(
themed_app: Page, assert_snapshot: ImageCompareFunction
):
"""Test that st.bar_chart renders with different theming."""
bar_chart_elements = themed_app.get_by_test_id("stArrowVegaLiteChart")
bar_chart_elements = themed_app.get_by_test_id("stVegaLiteChart")
expect(bar_chart_elements).to_have_count(TOTAL_BAR_CHARTS)

# Also make sure that all canvas objects are rendered:
expect(bar_chart_elements.locator("canvas")).to_have_count(TOTAL_BAR_CHARTS)

# Only test a single chart per built-in chart type:
assert_snapshot(bar_chart_elements.nth(1), name="st_bar_chart_themed")


def test_check_top_level_class(app: Page):
"""Check that the top level class is correctly set."""
check_top_level_class(app, "stVegaLiteChart")
Loading

0 comments on commit e91bea8

Please sign in to comment.