Allow callback for "dotStyle" and "activeDotStyle" props #828

sarimarton · 2022-04-22T22:15:28Z

I have to use rc-slider to make a range selector, where the active range has a horizontal color gradient. I can style the trackline with pure CSS, but not the step dots. I even went to great extents with CSS by adding some creative attribute selectors based on the style="left: ..." values, which was enough to have the correct dot colors when the full range is selected. But when the range is set to a narrower state, the gradient changes too, and the dots can't adapt to that. I can only solve this with a callback where I can set the style based on the dot position and the slider value. This is what this PR is about.

I have to use rc-slider to make a range selector, where the active range has a horizontal color gradient. I can style the trackline with pure CSS, but not the step dots. I even went to great extents with CSS by adding some creative attribute selectors based on the `style="left: ..."` values, which was enough to have the correct dot colors when the full range is selected. But when the range is set to a narrower state, the gradient changes too, and the dots can't adapt to that. I can only solve this with a callback where I can set the style based on the dot position and the slider value. This is what this PR is about.

vercel · 2022-04-22T22:15:32Z

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

Name	Status	Preview	Updated
slider	✅ Ready (Inspect)	Visit Preview	Apr 23, 2022 at 5:08PM (UTC)

yoyo837 · 2022-04-23T04:38:33Z

Please change the type definition, and add some test case for this.

codecov · 2022-04-23T04:40:20Z

Codecov Report

Merging #828 (0168d82) into master (410717a) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #828   +/-   ##
=======================================
  Coverage   99.81%   99.81%           
=======================================
  Files          13       13           
  Lines         533      535    +2     
  Branches      135      137    +2     
=======================================
+ Hits          532      534    +2     
  Misses          1        1

Impacted Files	Coverage Δ
src/Slider.tsx	`100.00% <ø> (ø)`
src/Steps/index.tsx	`100.00% <ø> (ø)`
src/Steps/Dot.tsx	`100.00% <100.00%> (ø)`

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

sarimarton · 2022-04-23T12:30:08Z

Please change the type definition, and add some test case for this.

I've added the types. As for the tests, I can't run the test suite because of an error which I couldn't fix, even after googling around. I'm pasting the output below. I've tried it on Windows and macOS.

~/repos.nosync/slider (patch-1) $ npm run test        

> rc-slider@10.0.0 test
> father test

 FAIL  tests/Slider.test.js
  ● Test suite failed to run

    TypeError: this._environment.runScript is not a function

      at Runtime._execModule (node_modules/umi-test/node_modules/jest-runtime/build/index.js:856:41)

 FAIL  tests/marks.test.js
  ● Test suite failed to run

    TypeError: this._environment.runScript is not a function

      at Runtime._execModule (node_modules/umi-test/node_modules/jest-runtime/build/index.js:856:41)

 FAIL  tests/common.test.js
  ● Test suite failed to run

    TypeError: this._environment.runScript is not a function

      at Runtime._execModule (node_modules/umi-test/node_modules/jest-runtime/build/index.js:856:41)

 FAIL  tests/Range.test.js
  ● Test suite failed to run

    TypeError: this._environment.runScript is not a function

      at Runtime._execModule (node_modules/umi-test/node_modules/jest-runtime/build/index.js:856:41)

Test Suites: 4 failed, 4 total
Tests:       0 total
Snapshots:   0 total
Time:        0.54s
Ran all test suites.
✖  error     Error: Jest failed 
    at /Users/sarimarton/repos.nosync/slider/node_modules/umi-test/lib/index.js:143:16
    at processTicksAndRejections (node:internal/process/task_queues:96:5)

lgtm-com · 2022-04-23T12:30:16Z

This pull request introduces 4 alerts when merging cbc16b2 into 410717a - view on LGTM.com

new alerts:

4 for Ineffective parameter type

yann-combarnous · 2022-04-23T13:17:12Z

Please change the type definition, and add some test case for this.

I've added the types. As for the tests, I can't run the test suite because of an error which I couldn't fix, even after googling around. I'm pasting the output below. I've tried it on Windows and macOS.

~/repos.nosync/slider (patch-1) $ npm run test        

> rc-slider@10.0.0 test
> father test

 FAIL  tests/Slider.test.js
  ● Test suite failed to run

    TypeError: this._environment.runScript is not a function

      at Runtime._execModule (node_modules/umi-test/node_modules/jest-runtime/build/index.js:856:41)

 FAIL  tests/marks.test.js
  ● Test suite failed to run

    TypeError: this._environment.runScript is not a function

      at Runtime._execModule (node_modules/umi-test/node_modules/jest-runtime/build/index.js:856:41)

 FAIL  tests/common.test.js
  ● Test suite failed to run

    TypeError: this._environment.runScript is not a function

      at Runtime._execModule (node_modules/umi-test/node_modules/jest-runtime/build/index.js:856:41)

 FAIL  tests/Range.test.js
  ● Test suite failed to run

    TypeError: this._environment.runScript is not a function

      at Runtime._execModule (node_modules/umi-test/node_modules/jest-runtime/build/index.js:856:41)

Test Suites: 4 failed, 4 total
Tests:       0 total
Snapshots:   0 total
Time:        0.54s
Ran all test suites.
✖  error     Error: Jest failed 
    at /Users/sarimarton/repos.nosync/slider/node_modules/umi-test/lib/index.js:143:16
    at processTicksAndRejections (node:internal/process/task_queues:96:5)

Your types are wrong I believe, should be:

React.CSSProperties | ((value: number) => React.CSSProperties);

See the related CI alert from LGTM in comment above.

You are also missing types updates in Slider.tsx for activeDotStyle and dotStyle props.

sarimarton · 2022-04-23T19:05:05Z

You are also missing types updates in Slider.tsx for activeDotStyle and dotStyle props.

Thanks, @yann-combarnous, for the suggestions.

yann-combarnous · 2022-04-25T08:20:12Z

@yoyo837, I followed the steps for installation and test commands from the README, and I am getting the same errors as the PR author.

So looks like some step is missing in the installation / contribution instructions. Any hint? I also removed any global Jest installation, as some suggested online.

npm@8.1.2
node@16.13.2

yoyo837 · 2022-04-25T08:22:14Z

README.md

@@ -97,8 +97,8 @@ The following APIs are shared by Slider and Range.
 | handleStyle | Array[Object] \| Object | `[{}]` | The style used for handle. (`both for slider(`Object`) and range(`Array of Object`), the array will be used for multi handle following element order`) |
 | trackStyle | Array[Object] \| Object | `[{}]` | The style used for track. (`both for slider(`Object`) and range(`Array of Object`), the array will be used for multi track following element order`)|
 | railStyle | Object | `{}` | The style used for the track base color.  |
-| dotStyle | Object | `{}` | The style used for the dots. |
-| activeDotStyle | Object | `{}` | The style used for the active dots. |
+| dotStyle | Object or (dotValue) => Object | `{}` | The style used for the dots. |


Suggested change

| dotStyle | Object or (dotValue) => Object | `{}` | The style used for the dots. |

| dotStyle | Object \| (dotValue) => Object | `{}` | The style used for the dots. |

yoyo837 · 2022-04-25T08:22:37Z

README.md

-| dotStyle | Object | `{}` | The style used for the dots. |
-| activeDotStyle | Object | `{}` | The style used for the active dots. |
+| dotStyle | Object or (dotValue) => Object | `{}` | The style used for the dots. |
+| activeDotStyle | Object or (dotValue) => Object | `{}` | The style used for the active dots. |


Suggested change

| activeDotStyle | Object or (dotValue) => Object | `{}` | The style used for the active dots. |

| activeDotStyle | Object \| (dotValue) => Object | `{}` | The style used for the active dots. |

yoyo837 · 2022-04-25T11:58:40Z

The failed does not seem to be caused by PR.

yann-combarnous · 2022-06-20T09:54:23Z

Thx @yoyo837 , any chance a new release can be made to include this feature?

vercel bot deployed to Preview April 22, 2022 22:18 View deployment

Add types

cbc16b2

vercel bot deployed to Preview April 23, 2022 12:30 View deployment

sarimarton added 2 commits April 23, 2022 18:53

Fix wrong typing

d293e0c

Add missing types to Slider.tsx

0168d82

vercel bot deployed to Preview April 23, 2022 17:08 View deployment

yoyo837 reviewed Apr 25, 2022

View reviewed changes

yoyo837 mentioned this pull request Apr 29, 2022

Allow callback for "dotStyle" and "activeDotStyle" props - with tests #830

Merged

4 tasks

yoyo837 closed this in #830 Jun 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow callback for "dotStyle" and "activeDotStyle" props #828

Allow callback for "dotStyle" and "activeDotStyle" props #828

sarimarton commented Apr 22, 2022

vercel bot commented Apr 22, 2022 •

edited

Loading

yoyo837 commented Apr 23, 2022

codecov bot commented Apr 23, 2022 •

edited

Loading

sarimarton commented Apr 23, 2022

lgtm-com bot commented Apr 23, 2022

yann-combarnous commented Apr 23, 2022 •

edited

Loading

sarimarton commented Apr 23, 2022

yann-combarnous commented Apr 25, 2022 •

edited

Loading

yoyo837 Apr 25, 2022

yoyo837 Apr 25, 2022

yoyo837 commented Apr 25, 2022

yann-combarnous commented Jun 20, 2022

	\| dotStyle \| Object or (dotValue) => Object \| `{}` \| The style used for the dots. \|
	\| dotStyle \| Object \\| (dotValue) => Object \| `{}` \| The style used for the dots. \|

	\| activeDotStyle \| Object or (dotValue) => Object \| `{}` \| The style used for the active dots. \|
	\| activeDotStyle \| Object \\| (dotValue) => Object \| `{}` \| The style used for the active dots. \|

Allow callback for "dotStyle" and "activeDotStyle" props #828

Allow callback for "dotStyle" and "activeDotStyle" props #828

Conversation

sarimarton commented Apr 22, 2022

vercel bot commented Apr 22, 2022 • edited Loading

yoyo837 commented Apr 23, 2022

codecov bot commented Apr 23, 2022 • edited Loading

Codecov Report

sarimarton commented Apr 23, 2022

lgtm-com bot commented Apr 23, 2022

yann-combarnous commented Apr 23, 2022 • edited Loading

sarimarton commented Apr 23, 2022

yann-combarnous commented Apr 25, 2022 • edited Loading

yoyo837 Apr 25, 2022

Choose a reason for hiding this comment

yoyo837 Apr 25, 2022

Choose a reason for hiding this comment

yoyo837 commented Apr 25, 2022

yann-combarnous commented Jun 20, 2022

vercel bot commented Apr 22, 2022 •

edited

Loading

codecov bot commented Apr 23, 2022 •

edited

Loading

yann-combarnous commented Apr 23, 2022 •

edited

Loading

yann-combarnous commented Apr 25, 2022 •

edited

Loading