search

VEuPathDB / web-components

Library of React components for plotting data
Apache License 2.0
1 stars 0 forks source link

implementing reverse slider feature #450

Open moontrip opened 1 year ago

moontrip commented 1 year ago

This is related to https://github.com/VEuPathDB/web-eda/issues/1605.

I missed the latest comment on the ticket so I thought we needed a reverse slider. This is the reason why I implemented a new feature, reverse slider, which can be used if necessary. It is not bad to have this feature, I suppose 😄

To utilize a reverse slider, two settings should be changed. a) isReverseSlider: true (default is false) b) change trackGradientStart and trackGradientEnd to reflect appropriate colors

Added a story at Slider.stories.tsx to take a look at how it works.

A screenshot is attached. reversed slider01

moontrip commented 1 year ago

@jernestmyers if you have time, it would be great if you can review this PR 👍

moontrip commented 1 year ago

I'm struggling to think of a use-case for this reversed state. To that end, I'd rather defer to other folks to see if they can imagine a use-case for these changes. If not, then I'd suggest we scrap it. If so, we can approve and move forward.

@jernestmyers Thank you for your review and suggestion! 👍 Actually the original ticket strongly requested to have the reverse slider for marker opacity: see this (https://github.com/VEuPathDB/web-eda/issues/1487#issuecomment-1380784054) and https://github.com/VEuPathDB/web-eda/issues/1605. Things were changed during I suffered from sickness in the end of Jan. that Danica just wanted to remain the slider as is. I found her comment late, after implementing the feature of reverse slider.

Honestly I am not quite sure whether they want to go back to the reverse slider or not. If you think this is overkill, then perhaps we can just switch this PR to draft one. The corresponding web-eda PR (https://github.com/VEuPathDB/web-eda/pull/1606) does not necessarily need this PR if a normal slider is used.

jernestmyers commented 1 year ago

Yeah, I think we should ask those involved in the request if they can envision us wanting this option in the future. The code itself looks great, I just want to be sure we have a need for this behavior first!

moontrip commented 1 year ago

Yeah, I think we should ask those involved in the request if they can envision us wanting this option in the future. The code itself looks great, I just want to be sure we have a need for this behavior first!

Sounds great @jernestmyers 👍 Perhaps we can leave this as is for the time being as this is not essential part of b62 anyway 😄 I will ask them about it at the original ticket's comment