Skip to content

ref(replays): Remove unneeded TimelinePosition and fold its css into scrubber.tsx#36952

Merged
ryan953 merged 3 commits into
masterfrom
ryan953/replay-timeline-colors
Jul 22, 2022
Merged

ref(replays): Remove unneeded TimelinePosition and fold its css into scrubber.tsx#36952
ryan953 merged 3 commits into
masterfrom
ryan953/replay-timeline-colors

Conversation

@ryan953

@ryan953 ryan953 commented Jul 21, 2022

Copy link
Copy Markdown
Member

There are two components to talk about, the Timeline (top part of screen shot) and Scrubber (lower), lets take them each in turn.

Here's the Before/After:

Before After
Mouse @ ~2:15 (scrubber being hovered) Screen Shot 2022-07-22 at 9 36 41 AM Screen Shot 2022-07-22 at 9 36 29 AM
Mouse @ ~5:15 (scrubber being hovered) Screen Shot 2022-07-22 at 9 35 54 AM Screen Shot 2022-07-22 at 9 53 16 AM
Mouse @ ~5:15 (timeline being hovered) Screen Shot 2022-07-22 at 9 47 59 AM Screen Shot 2022-07-22 at 9 47 45 AM
  1. Timeline

  2. Scrubber

    • Change background leading to the currentTime is purple200 (was purple100)
      • When the round currentTime nub was not visible, the purple200 blends into the gray background too much imo
      • The background here was purple300 before fix(replays): timeline scrubber colors #36853, i think moving it to 100 was an accident
    • Keep background leading to the mouseTime is still transparent
    • Keep rectangle to mark the mouseTime is still present
  3. Refactors

    • Don't need <TimelinePosition>. It was setting the border-right property for us, but now it's part of scrubber.tsx. Save a few dom nodes.

Fixes #36815
Fixes #36821

@ryan953 ryan953 requested a review from a team July 21, 2022 22:37
@github-actions github-actions Bot added the Scope: Frontend Automatically applied to PRs that change frontend components label Jul 21, 2022
@ryan953

ryan953 commented Jul 21, 2022

Copy link
Copy Markdown
Member Author

cc @Jesse-Box I'm proposing some timeline color changes here, and fixing scrubber colors.

@Jesse-Box Jesse-Box left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hey man, I think you raise some good points here, and although I think the purple200 is too distracting for me personally, I understand you're trying to find the best for both cases.

Did you try replacing the scrubber's cursor style for the timeline one

@ryan953

ryan953 commented Jul 22, 2022

Copy link
Copy Markdown
Member Author

Hey man, I think you raise some good points here, and although I think the purple200 is too distracting for me personally, I understand you're trying to find the best for both cases.

Did you try replacing the scrubber's cursor style for the timeline one

Yeah purple200 is a big step up :( in both spots. They don’t have to be the same though!

For me in the timeline, without a border on the edge purple100 is ok against the white background, we could add a right border there to match the mouse cursor tracking. Then it’s like your moving that bar along.
But the scrubber with 200 on top the gray is really better imo. The scrubber I think still needs that rectangle thing, so it is visible when you hover to the left of the current time.

I think with the timeline when we made the purple area taller it kinda brought some stuff along for the ride, for example the overlapping transparencies from before.

@ryan953 ryan953 force-pushed the ryan953/replay-timeline-colors branch from 2bbd7e7 to a82e85b Compare July 22, 2022 16:55
@ryan953 ryan953 merged commit cdd3d00 into master Jul 22, 2022
@ryan953 ryan953 deleted the ryan953/replay-timeline-colors branch July 22, 2022 18:02
@github-actions github-actions Bot locked and limited conversation to collaborators Aug 7, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Scope: Frontend Automatically applied to PRs that change frontend components

Projects

None yet

2 participants