Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Shared button component #10347

Merged
merged 3 commits into from
Feb 23, 2024
Merged

Shared button component #10347

merged 3 commits into from
Feb 23, 2024

Conversation

bvaughn
Copy link
Contributor

@bvaughn bvaughn commented Feb 21, 2024

The ui/components/shared/Button component has long confused me. It exports a lot of component variants, many of which don't seem to be used directly (rather the base Button component is used with various combinations of props) and many of which don't actually work (combinations of attributes that don't produce usable styles). In addition to that, most of our newer DevTools code can't use these buttons because the replay-next package can't import from the legacy ui package.

This PR adds a new Button component to replay-next that supports our two style variants (solid vs outlined) and colors (primary vs secondary). The API is more constrained (e.g. it doesn't try to support a dozen colors, when we only use two and it doesn't export a dozen components when there is, conceptually, only one button).

cc @jonbell-lot23 since this is a UI related change.

Comparison screenshots

Here is a side by side comparison of the old and new buttons.

Dark theme Light theme
Screenshot 2024-02-22 at 10 06 52 AM Screenshot 2024-02-22 at 10 07 02 AM

The new button component fixes some issues with the old one, some of which can be seen in the comparison above:

  • Legacy variants that should work, but don't (like secondary + outline)
  • Legacy hover states that don't work (secondary)
  • Disabled colors for dark theme
  • Add disabled state for outline variant
The screenshots above were generated using the following test code.
import { Button } from "replay-next/components/Button";
import {
  DisabledButton,
  DisabledLgButton,
  DisabledSmButton,
  PrimaryButton,
  PrimaryLgButton,
  PrimarySmButton,
  SecondaryButton,
  SecondaryLgButton,
  SecondarySmButton,
} from "ui/components/shared/Button";

export default function Test() {
  return (
    <div className="flex h-full w-full flex-col items-center overflow-auto p-1">
      <div className="grid auto-cols-auto grid-cols-4 gap-1">
        <div>Old</div>
        <div>New</div>

        <div>Old</div>
        <div>New</div>

        {/* Large */}
        <div>
          <PrimaryLgButton color="blue">Primary</PrimaryLgButton>
        </div>
        <div>
          <Button color="primary" size="large" variant="solid">
            Primary
          </Button>
        </div>
        <div>
          <PrimaryLgButton color="pink">Secondary</PrimaryLgButton>
        </div>
        <div>
          <Button color="secondary" size="large" variant="solid">
            Secondary
          </Button>
        </div>
        <div>
          <DisabledLgButton>Primary</DisabledLgButton>
        </div>
        <div>
          <Button color="primary" disabled size="large" variant="solid">
            Primary
          </Button>
        </div>
        <div>
          <DisabledLgButton>Secondary</DisabledLgButton>
        </div>
        <div>
          <Button color="secondary" disabled size="large" variant="solid">
            Secondary
          </Button>
        </div>
        <div>
          <SecondaryLgButton color="blue">Primary</SecondaryLgButton>
        </div>
        <div>
          <Button color="primary" size="large" variant="outline">
            Primary
          </Button>
        </div>
        <div>
          <SecondaryLgButton color="pink">Secondary</SecondaryLgButton>
        </div>
        <div>
          <Button color="secondary" size="large" variant="outline">
            Secondary
          </Button>
        </div>
        <div>
          <DisabledLgButton>Primary</DisabledLgButton>
        </div>
        <div>
          <Button color="primary" disabled size="large" variant="outline">
            Primary
          </Button>
        </div>
        <div>
          <DisabledLgButton>Secondary</DisabledLgButton>
        </div>
        <div>
          <Button color="secondary" disabled size="large" variant="outline">
            Secondary
          </Button>
        </div>

        {/* Regular */}
        <div>
          <PrimaryButton color="blue">Primary</PrimaryButton>
        </div>
        <div>
          <Button color="primary" variant="solid">
            Primary
          </Button>
        </div>
        <div>
          <PrimaryButton color="pink">Secondary</PrimaryButton>
        </div>
        <div>
          <Button color="secondary" variant="solid">
            Secondary
          </Button>
        </div>
        <div>
          <DisabledButton>Primary</DisabledButton>
        </div>
        <div>
          <Button color="primary" disabled variant="solid">
            Primary
          </Button>
        </div>
        <div>
          <DisabledButton>Secondary</DisabledButton>
        </div>
        <div>
          <Button color="secondary" disabled variant="solid">
            Secondary
          </Button>
        </div>
        <div>
          <SecondaryButton color="blue">Primary</SecondaryButton>
        </div>
        <div>
          <Button color="primary" variant="outline">
            Primary
          </Button>
        </div>
        <div>
          <SecondaryButton color="pink">Secondary</SecondaryButton>
        </div>
        <div>
          <Button color="secondary" variant="outline">
            Secondary
          </Button>
        </div>
        <div>
          <DisabledButton>Primary</DisabledButton>
        </div>
        <div>
          <Button color="primary" disabled variant="outline">
            Primary
          </Button>
        </div>
        <div>
          <DisabledButton>Secondary</DisabledButton>
        </div>
        <div>
          <Button color="secondary" disabled variant="outline">
            Secondary
          </Button>
        </div>

        {/* Small */}
        <div>
          <PrimarySmButton color="blue">Primary</PrimarySmButton>
        </div>
        <div>
          <Button color="primary" size="small" variant="solid">
            Primary
          </Button>
        </div>
        <div>
          <PrimarySmButton color="pink">Secondary</PrimarySmButton>
        </div>
        <div>
          <Button color="secondary" size="small" variant="solid">
            Secondary
          </Button>
        </div>
        <div>
          <DisabledSmButton>Primary</DisabledSmButton>
        </div>
        <div>
          <Button color="primary" disabled size="small" variant="solid">
            Primary
          </Button>
        </div>
        <div>
          <DisabledSmButton>Secondary</DisabledSmButton>
        </div>
        <div>
          <Button color="secondary" disabled size="small" variant="solid">
            Secondary
          </Button>
        </div>
        <div>
          <SecondarySmButton color="blue">Primary</SecondarySmButton>
        </div>
        <div>
          <Button color="primary" size="small" variant="outline">
            Primary
          </Button>
        </div>
        <div>
          <SecondarySmButton color="pink">Secondary</SecondarySmButton>
        </div>
        <div>
          <Button color="secondary" size="small" variant="outline">
            Secondary
          </Button>
        </div>
        <div>
          <DisabledSmButton>Primary</DisabledSmButton>
        </div>
        <div>
          <Button color="primary" disabled size="small" variant="outline">
            Primary
          </Button>
        </div>
        <div>
          <DisabledSmButton>Secondary</DisabledSmButton>
        </div>
        <div>
          <Button color="secondary" disabled size="small" variant="outline">
            Secondary
          </Button>
        </div>
      </div>
    </div>
  );
}

Example replacements

Below I've updated a couple of pages to use the new button component instead.

Before After
Screenshot 2024-02-22 at 10 07 25 AM Screenshot 2024-02-22 at 10 07 44 AM
Screenshot 2024-02-22 at 10 07 32 AM Screenshot 2024-02-22 at 10 07 50 AM
Screenshot 2024-02-22 at 10 07 38 AM Screenshot 2024-02-22 at 10 08 01 AM
Screenshot 2024-02-22 at 10 58 38 AM Screenshot 2024-02-22 at 10 58 42 AM
Screenshot 2024-02-22 at 10 59 24 AM Screenshot 2024-02-22 at 10 59 28 AM
Screenshot 2024-02-22 at 10 59 39 AM Screenshot 2024-02-22 at 10 59 44 AM

Note that by updating this page we fix the following problems:

  • Bright disabled button style on a dark theme page
  • Bright background on outline button (bad contrast– blue on white)
  • "Done" button is overlapped/hidden entirely when selecting elements.
  • Generally simplifies the code required to use our standard button UI/UX
  • Removes the only instance of a "red" button color in favor of a standard color; (this might be controversial and we could change this if needed)

Follow up work to this PR

  • Update replay-next code to use these buttons instead of one-off re-implementing the styles everywhere
  • Gradually remove references to ui/components/shared/Button and replace with new buttons
  • Delete all ui/components/shared/Button components once references have been removed

Copy link

vercel bot commented Feb 21, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
devtools ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 22, 2024 4:03pm

Copy link

replay-io bot commented Feb 21, 2024

E2E Tests

Status Complete ↗︎
Commit 9d3e333
Results
⚠️ 1 Flaky
  • repaint-01: repaints the screen screen when stepping over code that modifies the DOM (Replay 1, Replay 2)
  • 107 Passed
  • authenticated/comments-01: Test add, edit, and delete comment functionality
  • authenticated/comments-02: Test shared comments and replies (Replay 1, Replay 2)
  • authenticated/comments-03: Comment previews
  • authenticated/logpoints-01: Shared logpoints functionality (Replay 1, Replay 2)
  • authenticated/new-test-suites/tests-01: basic tests
  • authenticated/new-test-suites/tests-02: test with no recording
  • authenticated/new-test-suites/tests-03: test ID in the URL
  • authenticated/passport-01: Time travel
  • authenticated/passport-02: Infrared inspection
  • authenticated/passport-03: Swiss army knife
  • authenticated/passport-04: Multiplayer
  • breakpoints-01: Test basic breakpoint functionality
  • breakpoints-02: Test unhandled divergence while evaluating at a breakpoint
  • breakpoints-03: Test stepping forward through breakpoints when rewound before the first one
  • breakpoints-04: catch, finally, generators, and async/await
  • breakpoints-05: Test interaction of breakpoints with debugger statements
  • breakpoints-06: Test log point in a sourcemapped file
  • breakpoints-07: rewind and seek using command bar and console messages
  • breakpoints-08: should be temporarily disabled
  • console_async: support console evaluations in async frames
  • console_dock: Should show the correct docking behavior for recordings with video
  • console_errors: Test that errors and warnings from various sources are shown in the console
  • console_eval: support console evaluations
  • console_warp-01: should support warping to console messages
  • console_warp-02: support pausing, warping, stepping and evaluating console messages
  • console-expressions-01: should cache input eager eval and terminal expressions per instance
  • cypress-01: Basic Test Suites panel functionality
  • cypress-02: Test Step timeline behavior
  • cypress-03: Test Step interactions
  • cypress-04: Test Step buttons and menu item
  • cypress-05: Test DOM node preview on user action step hover
  • deleted-recording: Show error message for deleted recording
  • elements-search: Element panel should support basic and advanced search modes
  • fe-1875 :: verify that steps go to the right point in time
  • file-search-01: should search files
  • focus_mode-01: should filter messages as regions based on the active focus mode
  • highlighter: element highlighter works everywhere
  • inspector-computed-01: Basic computed styles can be viewed
  • inspector-elements-01: Basic DOM tree node display
  • inspector-elements-02_node-picker: element picker and iframe behavior
  • inspector-elements-03: Nested node picker and selection behavior
  • inspector-elements-04: Keyboard shortcuts should select the right DOM nodes
  • inspector-elements-05_search: element picker and iframe behavior
  • inspector-rules-01: Basic CSS rules should be viewed
  • inspector-rules-02: Sourcemapped rules should be viewed
  • inspector-rules-03: Shorthand CSS rules should be viewed
  • jump-to-code-01: Test basic jumping functionality
  • logpoints-01: log-points appear in the correct order and allow time warping
  • logpoints-02: conditional log-points
  • logpoints-03: should display event properties in the console
  • logpoints-04: should display exceptions in the console
  • logpoints-05: should auto-complete based on log point location
  • logpoints-06: should be temporarily disabled
  • logpoints-07: should use the correct scope in auto-complete
  • logpoints-08: should support jumping directly to a hit point via the capsule input
  • logpoints-09: should support pending edits
  • logpoints-10: too-many-points-to-find UX
  • logpoints-11: too-many-points-to-run-analysis UX
  • network-01: should filter requests by type and text
  • network-02: should show details for the selected request
  • network-03: should sync and display the current time in relation to the network requests
  • node_console_dock: Should show the correct docking behavior for recordings without video
  • node_console-01: Basic node console behavior
  • node_console-02: uncaught exceptions should show up
  • node_control_flow: catch, finally, generators, and async/await
  • node_logpoint-01: Basic node logpoints
  • node_object_preview: Showing console objects in node
  • node_quick_open_modal-01: Test basic searching functionality
  • node_spawn: Basic subprocess spawning
  • node_stepping-01: Test stepping in async frames and async call stacks
  • node_worker-01: make sure node workers don't cause crashes
  • object_preview-01: expressions in the console after time warping
  • object_preview-02: should allow objects in scope to be inspected
  • object_preview-03: Test previews when switching between frames and stepping
  • object_preview-04: Test scope mapping and switching between generated/original sources
  • object_preview-05: Should support logging objects as values
  • object_preview-06: HTML elements
  • playwright-01: Basic Test Suites panel functionality
  • playwright-02: Test Step timeline behavior
  • playwright-03: Test Step interactions
  • playwright-04: Test Step buttons and menu item
  • playwright-05: Test DOM node previews on user action step hover
  • react_devtools-01: Basic RDT behavior
  • react_devtools-02: RDT integrations (Chromium)
  • react_devtools-03: process and display multiple React versions in page
  • react_devtools-04: Component selection is maintained when seeking to a new point
  • redux_devtools: Test Redux DevTools.
  • repaint-02: repaints on hover
  • repaint-03: repaints on seek
  • resizable-panels-01: Left side Toolbar and Video should be collapsible
  • restart debugging session
  • scopes_rerender: Test that scopes are rerendered
  • source-line-highlights: Test source line highlighting
  • sourcemap_stacktrace: Test that stacktraces are sourcemapped
  • stacking: Element highlighter selects the correct element when they overlap
  • stepping-01: Test basic step-over/back functionality
  • stepping-02: Test fixes for some simple stepping bugs
  • stepping-03: Stepping past the beginning or end of a frame should act like a step-out
  • stepping-04: Test stepping in a frame other than the top frame
  • stepping-05_chromium: Test stepping in pretty-printed code
  • stepping-05: Test stepping in pretty-printed code
  • stepping-06: Test stepping in async frames and async call stacks
  • stepping-07: Test quick stepping using the keyboard
  • test-suite-dashboard/test-runs-01: passed run in main branch with source
  • test-suite-dashboard/test-runs-02: failed run in temp branch without source
  • test-suite-dashboard/test-runs-03: flaky run in main branch with source
  • test-suite-dashboard/test-runs-04: test ID in the URL
  • @@ -0,0 +1,58 @@
    .Button {
    font-family: var(--font-family-default);
    font-size: 0.875rem;
    Copy link
    Contributor Author

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    I don't like using non-standard font sizes and pixel sizes here, but I matched the old styles exactly so this switch would be less controversial.

    @bvaughn bvaughn changed the title Shared button component (WIP) Shared button component Feb 22, 2024
    @bvaughn bvaughn merged commit 1e1620f into main Feb 23, 2024
    24 checks passed
    @bvaughn bvaughn deleted the FE-2293 branch February 23, 2024 16:01
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    None yet
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    2 participants