Refactor frontend styles, class names and test-ids - Part 4 (streamli…

…t#9320) ## Describe your changes Applies a couple of small styling refactorings and adds/unifies classnames & data-testid's for: - [x] app / AppView - [x] app / EventContainer - [x] app / Header - [x] app / MainMenu - [x] app / Sidebar - [x] app / ToolbarActions - [x] app / StatusWidget - [x] lib / Block 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. --------- Co-authored-by: Lukas Masuch <[email protected]>
souvikmaji · Aug 29, 2024 · 621b802 · 621b802
1 parent 7450582
commit 621b802
Show file tree

Hide file tree

Showing 36 changed files with 212 additions and 144 deletions.
diff --git a/e2e/specs/hostframe.spec.js b/e2e/specs/hostframe.spec.js
@@ -63,7 +63,7 @@ describe("Host communication", () => {
       // Open the Main Menu
       cy.get("#MainMenu > button").click();
       // Open the Settings Modal
-      cy.getIndexed('[data-testid="main-menu-list"] > ul', 1).click({
+      cy.getIndexed('[data-testid="stMainMenuList"] > ul', 1).click({
         force: true,
       });
       cy.get("div[role='dialog']").should("exist");
@@ -83,12 +83,12 @@ describe("Host communication", () => {
       // Open the Main Menu
       cy.get("#MainMenu > button").click();
       // Check that new menu item exists
-      cy.getIndexed('[data-testid="main-menu-list"] > ul', 4).should(
+      cy.getIndexed('[data-testid="stMainMenuList"] > ul', 4).should(
         "have.text",
         "Adopt a Corgi"
       );
       // Exit main menu
-      cy.get(".main").type("{esc}");
+      cy.get('[data-testid="stMain"]').type("{esc}");
     });
   });
 
@@ -97,12 +97,12 @@ describe("Host communication", () => {
     cy.get("#toolbar").contains("Add Toolbar Item").click();
     cy.get("iframe").iframe(() => {
       // Check toolbar contents
-      cy.get(".stActionButton").should("exist");
-      cy.getIndexed('[data-testid="stActionButton"]', 0).should(
+      cy.get(".stToolbarActionButton").should("exist");
+      cy.getIndexed('[data-testid="stToolbarActionButton"]', 0).should(
         "have.text",
         "Favorite"
       );
-      cy.getIndexed('[data-testid="stActionButton"]', 1).should(
+      cy.getIndexed('[data-testid="stToolbarActionButton"]', 1).should(
         "have.text",
         "Share"
       );

diff --git a/e2e_playwright/st_caption_test.py b/e2e_playwright/st_caption_test.py
@@ -64,7 +64,7 @@ def test_match_snapshot_for_caption_with_tooltip(
     """Test that the caption with matches the snapshot. Also test dark-theme to make
     sure icon is visible."""
     caption_container = (
-        themed_app.get_by_test_id("element-container")
+        themed_app.get_by_test_id("stElementContainer")
         .filter(has=themed_app.get_by_test_id("stCaptionContainer"))
         .nth(3)
     )
@@ -76,7 +76,7 @@ def test_match_snapshot_for_mixed_markdown_content(
 ):
     """Test that the big markdown caption with the mixed content matches the snapshot."""
     caption_container = (
-        app.get_by_test_id("element-container")
+        app.get_by_test_id("stElementContainer")
         .filter(has=app.get_by_test_id("stCaptionContainer"))
         .nth(4)
     )

diff --git a/e2e_playwright/st_chat_input_test.py b/e2e_playwright/st_chat_input_test.py
@@ -59,7 +59,7 @@ def test_embedded_app_with_bottom_chat_input(
     page.goto(f"http://localhost:{app_port}/?embed=true")
     wait_for_app_loaded(page, embedded=True)
 
-    app_view_block = page.get_by_test_id("stAppViewBlockContainer")
+    app_view_block = page.get_by_test_id("stMainBlockContainer")
     # Bottom padding should be 16px (1rem):
     expect(app_view_block).to_have_css("padding-bottom", "16px")
     bottom_block = page.get_by_test_id("stBottomBlockContainer")
@@ -69,9 +69,9 @@ def test_embedded_app_with_bottom_chat_input(
     expect(bottom_block).to_have_css("padding-top", "16px")
 
     # There shouldn't be an iframe resizer anchor:
-    expect(page.get_by_test_id("IframeResizerAnchor")).to_be_hidden()
+    expect(page.get_by_test_id("stAppIframeResizerAnchor")).to_be_hidden()
     # The scroll container should be switched to scroll to bottom:
-    expect(page.get_by_test_id("ScrollToBottomContainer")).to_be_attached()
+    expect(page.get_by_test_id("stAppScrollToBottomContainer")).to_be_attached()
 
     assert_snapshot(
         page.get_by_test_id("stAppViewContainer"),
@@ -81,7 +81,7 @@ def test_embedded_app_with_bottom_chat_input(
 
 def test_app_with_bottom_chat_input(app: Page, assert_snapshot: ImageCompareFunction):
     """Test that an app with bottom chat input renders correctly."""
-    app_view_block = app.get_by_test_id("stAppViewBlockContainer")
+    app_view_block = app.get_by_test_id("stMainBlockContainer")
     # Bottom padding should be 16px (1rem):
     expect(app_view_block).to_have_css("padding-bottom", "16px")
 
@@ -92,9 +92,9 @@ def test_app_with_bottom_chat_input(app: Page, assert_snapshot: ImageCompareFunc
     expect(bottom_block).to_have_css("padding-top", "16px")
 
     # There shouldn't be an iframe resizer anchor:
-    expect(app.get_by_test_id("IframeResizerAnchor")).to_be_hidden()
+    expect(app.get_by_test_id("stAppIframeResizerAnchor")).to_be_hidden()
     # The scroll container should be switched to scroll to bottom:
-    expect(app.get_by_test_id("ScrollToBottomContainer")).to_be_attached()
+    expect(app.get_by_test_id("stAppScrollToBottomContainer")).to_be_attached()
 
     assert_snapshot(app.get_by_test_id("stBottom"), name="st_chat_input-app_bottom")
 

diff --git a/e2e_playwright/st_expander_test.py b/e2e_playwright/st_expander_test.py
@@ -42,7 +42,7 @@ def test_expander_displays_correctly(
 
 def test_expander_collapses_and_expands(app: Page):
     """Test that an expander collapses and expands."""
-    main_container = app.get_by_test_id("stAppViewBlockContainer")
+    main_container = app.get_by_test_id("stMain")
     main_expanders = main_container.get_by_test_id("stExpander")
     expect(main_expanders).to_have_count(7)
 
@@ -73,7 +73,7 @@ def test_empty_expander_not_rendered(app: Page):
 
 def test_expander_session_state_set(app: Page):
     """Test that session state updates are propagated to expander content"""
-    main_container = app.get_by_test_id("stAppViewBlockContainer")
+    main_container = app.get_by_test_id("stMain")
     main_expanders = main_container.get_by_test_id("stExpander")
     expect(main_expanders).to_have_count(7)
 

diff --git a/e2e_playwright/st_markdown_test.py b/e2e_playwright/st_markdown_test.py
@@ -47,9 +47,7 @@ def test_displays_individual_markdowns(app: Page):
     """Verifies the correct text content of markdown elements."""
 
     # get markdown elements in main app view, not sidebar
-    markdown_elements = app.get_by_test_id("stAppViewBlockContainer").get_by_test_id(
-        "stMarkdown"
-    )
+    markdown_elements = app.get_by_test_id("stMain").get_by_test_id("stMarkdown")
 
     # Assert the text content of each markdown element
     text = [
@@ -187,9 +185,7 @@ def test_help_tooltip_works(app: Page):
     """Test that the help tooltip is displayed on hover."""
     # Get the first element in the main view:
     markdown_with_help = (
-        app.get_by_test_id("stAppViewBlockContainer")
-        .get_by_test_id("stMarkdown")
-        .nth(0)
+        app.get_by_test_id("stMain").get_by_test_id("stMarkdown").nth(0)
     )
     expect_help_tooltip(app, markdown_with_help, "This is a help tooltip!")
 

diff --git a/frontend/app/src/App.test.tsx b/frontend/app/src/App.test.tsx
@@ -2629,7 +2629,9 @@ describe("App", () => {
 
       sendForwardMessage("newSession", NEW_SESSION_JSON)
 
-      expect(screen.queryByTestId("stActionButton")).not.toBeInTheDocument()
+      expect(
+        screen.queryByTestId("stToolbarActionButton")
+      ).not.toBeInTheDocument()
 
       fireWindowPostMessage({
         type: "SET_TOOLBAR_ITEMS",
@@ -2641,7 +2643,7 @@ describe("App", () => {
         ],
       })
 
-      expect(screen.getByTestId("stActionButton")).toBeInTheDocument()
+      expect(screen.getByTestId("stToolbarActionButton")).toBeInTheDocument()
     })
 
     it("sets hideSidebarNav based on the server config option and host setting", () => {

diff --git a/frontend/app/src/components/AppView/AppView.test.tsx b/frontend/app/src/components/AppView/AppView.test.tsx
@@ -116,7 +116,9 @@ describe("AppView element", () => {
 
   it("renders without crashing", () => {
     render(<AppView {...getProps()} />)
-    expect(screen.getByTestId("stAppViewContainer")).toBeInTheDocument()
+    const appViewContainer = screen.getByTestId("stAppViewContainer")
+    expect(appViewContainer).toBeInTheDocument()
+    expect(appViewContainer).toHaveClass("stAppViewContainer")
   })
 
   it("does not render a sidebar when there are no elements and only one page", () => {
@@ -293,7 +295,7 @@ describe("AppView element", () => {
     render(<AppView {...props} />)
 
     const style = window.getComputedStyle(
-      screen.getByTestId("stAppViewBlockContainer")
+      screen.getByTestId("stMainBlockContainer")
     )
     expect(style.maxWidth).not.toEqual("initial")
   })
@@ -309,7 +311,7 @@ describe("AppView element", () => {
     })
     render(<AppView {...getProps()} />)
     const style = window.getComputedStyle(
-      screen.getByTestId("stAppViewBlockContainer")
+      screen.getByTestId("stMainBlockContainer")
     )
     expect(style.maxWidth).toEqual("initial")
   })
@@ -356,6 +358,7 @@ describe("AppView element", () => {
       expect(sourceSpy).toHaveBeenCalledWith(
         "https://docs.streamlit.io/logo.svg"
       )
+      expect(collapsedLogo).toHaveClass("stLogo")
     })
 
     it("defaults to image if no iconImage", () => {
@@ -414,7 +417,7 @@ describe("AppView element", () => {
     const props = getProps()
     render(<AppView {...props} />)
 
-    const stbContainer = screen.queryByTestId("ScrollToBottomContainer")
+    const stbContainer = screen.queryByTestId("stAppScrollToBottomContainer")
     expect(stbContainer).not.toBeInTheDocument()
   })
 
@@ -463,7 +466,7 @@ describe("AppView element", () => {
 
     render(<AppView {...props} />)
 
-    const stbContainer = screen.queryByTestId("ScrollToBottomContainer")
+    const stbContainer = screen.queryByTestId("stAppScrollToBottomContainer")
     expect(stbContainer).toBeInTheDocument()
   })
 })
diff --git a/frontend/app/src/components/AppView/AppView.tsx b/frontend/app/src/components/AppView/AppView.tsx
@@ -188,6 +188,7 @@ function AppView(props: AppViewProps): ReactElement {
         src={source}
         size={appLogo.size}
         alt="Logo"
+        className="stLogo"
         data-testid="stLogo"
       />
     )
@@ -263,11 +264,12 @@ function AppView(props: AppViewProps): ReactElement {
         tabIndex={0}
         isEmbedded={embedded}
         disableScrolling={disableScrolling}
-        className="stAppViewMain main"
+        className="stMain"
+        data-testid="stMain"
       >
         <StyledAppViewBlockContainer
-          className="stAppViewBlockContainer block-container"
-          data-testid="stAppViewBlockContainer"
+          className="stMainBlockContainer block-container"
+          data-testid="stMainBlockContainer"
           isWideMode={wideMode}
           showPadding={showPadding}
           addPaddingForHeader={showToolbar || showColoredLine}
@@ -284,7 +286,7 @@ function AppView(props: AppViewProps): ReactElement {
         well together. */}
         {!hasBottomElements && (
           <StyledIFrameResizerAnchor
-            data-testid="IframeResizerAnchor"
+            data-testid="stAppIframeResizerAnchor"
             data-iframe-height
           />
         )}
@@ -296,7 +298,10 @@ function AppView(props: AppViewProps): ReactElement {
            height in the scroll area. Thereby, the bottom container will never
            cover something if you scroll to the end.*/}
             <StyledAppViewBlockSpacer />
-            <StyledStickyBottomContainer data-testid="stBottom">
+            <StyledStickyBottomContainer
+              className="stBottom"
+              data-testid="stBottom"
+            >
               <StyledInnerBottomContainer>
                 <StyledBottomBlockContainer
                   data-testid="stBottomBlockContainer"
@@ -312,7 +317,7 @@ function AppView(props: AppViewProps): ReactElement {
       </Component>
       {hasEventElements && (
         <EventContainer scriptRunId={elements.event.scriptRunId}>
-          <StyledEventBlockContainer>
+          <StyledEventBlockContainer className="stEvent" data-testid="stEvent">
             {renderBlock(elements.event)}
           </StyledEventBlockContainer>
         </EventContainer>

diff --git a/frontend/app/src/components/AppView/ScrollToBottomContainer.tsx b/frontend/app/src/components/AppView/ScrollToBottomContainer.tsx
@@ -39,7 +39,7 @@ export default function ScrollToBottomContainer(props: Props): ReactElement {
       isEmbedded={isEmbedded}
       disableScrolling={disableScrolling}
       ref={scrollContainerRef}
-      data-testid="ScrollToBottomContainer"
+      data-testid="stAppScrollToBottomContainer"
     >
       {children}
     </StyledAppViewMain>

diff --git a/frontend/app/src/components/AppView/styled-components.ts b/frontend/app/src/components/AppView/styled-components.ts
@@ -128,7 +128,6 @@ export const StyledAppViewBlockContainer =
       }
       const bottomEmbedPadding =
         showPadding && !hasBottom ? "10rem" : theme.spacing.lg
-      const wideSidePadding = isWideMode ? "5rem" : theme.spacing.lg
 
       // Full screen-enabled elements can overflow the page when the screen
       // size is slightly over the content max width.
@@ -152,9 +151,13 @@ export const StyledAppViewBlockContainer =
         paddingLeft: theme.spacing.lg,
         paddingRight: theme.spacing.lg,
         // Increase side padding, if layout = wide and we're not on mobile
-        "@media (min-width: 576px)": {
-          paddingLeft: wideSidePadding,
-          paddingRight: wideSidePadding,
+        [`@media (min-width: ${theme.breakpoints.sm})`]: {
+          paddingLeft: isWideMode
+            ? theme.sizes.wideSidePadding
+            : theme.spacing.lg,
+          paddingRight: isWideMode
+            ? theme.sizes.wideSidePadding
+            : theme.spacing.lg,
         },
         paddingTop: topEmbedPadding,
         paddingBottom: bottomEmbedPadding,
@@ -190,23 +193,26 @@ export interface StyledBottomBlockContainerProps {
 export const StyledBottomBlockContainer =
   styled.div<StyledBottomBlockContainerProps>(
     ({ isWideMode, showPadding, theme }) => {
-      const wideSidePadding = isWideMode ? "5rem" : theme.spacing.lg
       return {
         width: theme.sizes.full,
         paddingLeft: theme.spacing.lg,
         paddingRight: theme.spacing.lg,
         // Increase side padding, if layout = wide and we're not on mobile
-        "@media (min-width: 576px)": {
-          paddingLeft: wideSidePadding,
-          paddingRight: wideSidePadding,
+        [`@media (min-width: ${theme.breakpoints.sm})`]: {
+          paddingLeft: isWideMode
+            ? theme.sizes.wideSidePadding
+            : theme.spacing.lg,
+          paddingRight: isWideMode
+            ? theme.sizes.wideSidePadding
+            : theme.spacing.lg,
         },
         paddingTop: theme.spacing.lg,
         paddingBottom: showPadding ? "55px" : theme.spacing.threeXL,
         minWidth: isWideMode ? "auto" : undefined,
         maxWidth: isWideMode ? "initial" : theme.sizes.contentMaxWidth,
 
         [`@media print`]: {
-          paddingTop: 0,
+          paddingTop: theme.spacing.none,
         },
       }
     }
@@ -219,7 +225,7 @@ export const StyledAppViewBlockSpacer = styled.div(({ theme }) => {
   }
 })
 
-export const StyledIFrameResizerAnchor = styled.div(() => ({
+export const StyledIFrameResizerAnchor = styled.div(({ theme }) => ({
   position: "relative",
-  bottom: "0",
+  bottom: theme.spacing.none,
 }))
diff --git a/frontend/app/src/components/EventContainer/EventContainer.test.tsx b/frontend/app/src/components/EventContainer/EventContainer.test.tsx
@@ -27,7 +27,8 @@ describe("EventContainer Component", () => {
   test("renders Toast Container", () => {
     render(<EventContainer scriptRunId="123" />)
 
-    const toastContainer = screen.getByTestId("toastContainer")
+    const toastContainer = screen.getByTestId("stToastContainer")
     expect(toastContainer).toBeInTheDocument()
+    expect(toastContainer).toHaveClass("stToastContainer")
   })
 })
diff --git a/frontend/app/src/components/EventContainer/EventContainer.tsx b/frontend/app/src/components/EventContainer/EventContainer.tsx
@@ -29,8 +29,8 @@ export interface EventContainerProps {
 function EventContainer({
   scriptRunId,
   children,
-}: EventContainerProps): ReactElement {
-  const { sizes }: EmotionTheme = useTheme()
+}: Readonly<EventContainerProps>): ReactElement {
+  const theme: EmotionTheme = useTheme()
 
   useEffect(() => {
     // Ensure all toasts cleared on script re-run
@@ -46,12 +46,12 @@ function EventContainer({
           Root: {
             style: {
               // Avoids blocking the header
-              top: sizes.headerHeight,
-              // Toasts overlap chatInput container
-              zIndex: 100,
+              top: theme.sizes.headerHeight,
+              zIndex: theme.zIndices.toast,
             },
             props: {
-              "data-testid": "toastContainer",
+              "data-testid": "stToastContainer",
+              className: "stToastContainer",
             },
           },
         }}