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

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

Merged
merged 2 commits into from
Mar 8, 2023

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
@codeclimate
Copy link

codeclimate bot commented Mar 8, 2023

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
Member

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
Member

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
Member

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