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.

Sign up for GitHub

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

Jump to bottom

fix: Slider step prop not accepting null value #26984

Merged
merged 3 commits into from
Oct 2, 2020
Merged

fix: Slider step prop not accepting null value #26984

merged 3 commits into from
Oct 2, 2020

Conversation

shangyilim
Copy link
Contributor

@shangyilim shangyilim commented Oct 1, 2020 •
edited
Loading

[中文版模板 / Chinese template]

🤔 This is a ...

  • New feature
  • Bug fix
  • Site / documentation update
  • Demo update
  • Component style update
  • TypeScript definition update
  • Bundle size optimization
  • Performance optimization
  • Enhancement feature
  • Internationalization
  • Refactoring
  • Code style optimization
  • Test Case
  • Branch merge
  • Other (about what?)

🔗 Related issue link

  1. #26976

💡 Background and solution

The step prop for Slider will trigger typescript error: Type null is not assignable to type 'number | undefined' . This is because the type definition does not allow null value, eventhough in the documents it allowed as mentioned:

the granularity the slider can step through values. Must greater than 0, and be divided by (max - min) . When marks no null, step can be null

This can be fixed by changing the definition of step to allow null value.
Since underlying component used in Slider is react-component/slider its step props do not allow null, although in their documentation it is allowed..
A quick fix will force the value null! within the library, while client apps will still be able to pass in step={null} without typescript errors. Until the underlying prop interface is resolved, this should a sufficient enough fix.

Updated typescript test to catch this error, and also added tests to ensure slider behaves as expected when null is passed.

📝 Changelog

No breaking changes

Language Changelog
🇺🇸 English Fixed step prop in Slider to accept null as a valid value in accordance to the documentation.
🇨🇳 Chinese

☑️ Self Check before Merge

⚠️ Please check all items below before review. ⚠️

  • Doc is updated/provided or not needed
  • Demo is updated/provided or not needed
  • TypeScript definition is updated/provided or not needed
  • Changelog is provided or not needed

@shangyilim shangyilim changed the title Fixes Slider step prop not accepting null value Fix: Slider step prop not accepting null value Oct 1, 2020
@ant-design-bot
Copy link
Contributor

ant-design-bot commented Oct 1, 2020 •
edited
Loading

@ant-design-bot
Copy link
Contributor

ant-design-bot commented Oct 1, 2020 •
edited
Loading

@codesandbox-ci
Copy link

codesandbox-ci bot commented Oct 1, 2020 •
edited
Loading

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 616570a:

Sandbox Source
antd reproduction template Configuration

@codecov
Copy link

codecov bot commented Oct 1, 2020 •
edited
Loading

Codecov Report

Merging #26984 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##            master    #26984   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          384       384           
  Lines         7370      7370           
  Branches      2051      2009   -42     
=========================================
  Hits          7370      7370
Impacted Files Coverage Δ
components/slider/index.tsx 100.00% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4f177dd...625ab01. Read the comment docs.

afc163
afc163 reviewed Oct 2, 2020
View reviewed changes
components/slider/index.tsx
@@ -159,6 +160,7 @@ const Slider = React.forwardRef<unknown, SliderSingleProps | SliderRangeProps>(
return (
<RcSlider
{...(restProps as SliderSingleProps)}
step={restProps.step!}
Copy link
Member

Choose a reason for hiding this comment

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

RcSlider 的定义也需要修正一下，避免用 !。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately until RcSlider fixes their definition, there is nothing we can do on our end and to avoid typescript lint errors.
Furthermore I have added another test to safeguard againts undefined:

it('step should not crash with undefined value', () => {
    [undefined, null].forEach(value => {
      mount(<Slider step={value} tooltipVisible />);
    });
  });

@afc163 afc163 changed the title Fix: Slider step prop not accepting null value fix: Slider step prop not accepting null value Oct 2, 2020
@afc163
Copy link
Member

afc163 commented Oct 2, 2020

image

Could you fill the changelog part?

@afc163 afc163 merged commit ef3aabd into ant-design:master Oct 2, 2020
@afc163 afc163 mentioned this pull request Oct 10, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@afc163 afc163 afc163 left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@shangyilim @ant-design-bot @afc163