Skip to content

Change the background color for the smooth slider over plots#3427

Merged
sroy3 merged 2 commits intomainfrom
smooth-slider-bg
Mar 8, 2023
Merged

Change the background color for the smooth slider over plots#3427
sroy3 merged 2 commits intomainfrom
smooth-slider-bg

Conversation

@sroy3
Copy link
Contributor

@sroy3 sroy3 commented Mar 8, 2023

Following #3405 (comment)

Screen.Recording.2023-03-08.at.2.29.08.PM.mov

@sroy3 sroy3 added the product PR that affects product label Mar 8, 2023
@sroy3 sroy3 self-assigned this Mar 8, 2023
@sroy3 sroy3 marked this pull request as ready for review March 8, 2023 19:29
@qlty-cloud-legacy
Copy link

Code Climate has analyzed commit 9ac9506 and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 100.0% (85% is the threshold).

This pull request will bring the total coverage in the repository to 95.7% (0.0% change).

View more on Code Climate.

@mattseddon
Copy link
Contributor

Up to you on whether or not to merge. Was a suggestion not a requirement.

@sroy3 sroy3 merged commit 7c93f89 into main Mar 8, 2023
@sroy3 sroy3 deleted the smooth-slider-bg branch March 8, 2023 20:51
@mattseddon
Copy link
Contributor

Looks weird for zoomed-in plot 🤦🏻:

image

@sroy3
Copy link
Contributor Author

sroy3 commented Mar 10, 2023

Looks weird for zoomed-in plot 🤦🏻:

image

Now (first) vs before (second) the change:

Screen.Recording.2023-03-10.at.9.50.03.AM.mov

It looks like it's actually better most of the time. If we want to make it look better, we'd need something different completely. Let's leave it like this for now as it's an improvement, and change it when we think of something better.

@shcheklein
Copy link
Contributor

I think the previous implementation was a bit better to be honest. I would try for these sliders to blend more with the plot itself. I don't see a strong reason for all sliders to have the same background, the context is different for them.

Sorry for going back and forth @sroy3 on this.

@sroy3
Copy link
Contributor Author

sroy3 commented Mar 10, 2023

I think the previous implementation was a bit better to be honest. I would try for these sliders to blend more with the plot itself. I don't see a strong reason for all sliders to have the same background, the context is different for them.

Sorry for going back and forth @sroy3 on this.

There are times when the slider is almost the same color as the background (see 0:42, 0:44, 0:48 in the video) (just making sure we're seeing the same thing). But I can easily revert and don't have an opinion on the matter.

@sroy3
Copy link
Contributor Author

sroy3 commented Mar 10, 2023

Reverted in #3444. Feel free to merge if needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

product PR that affects product

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants