Skip to content

Conversation

Kim-Wing-Fung
Copy link
Contributor

Added onSelect callback to RangePicker to match the functionality in PickerPanel.

@vercel
Copy link

vercel bot commented Jun 26, 2020

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/react-component/picker/5ifysxder
✅ Preview: https://picker-git-fork-kim-wing-fung-add-onselect-callb-67abb6.react-component.vercel.app

@codecov
Copy link

codecov bot commented Jun 26, 2020

Codecov Report

Merging #93 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #93   +/-   ##
=======================================
  Coverage   99.57%   99.57%           
=======================================
  Files          43       43           
  Lines        1900     1901    +1     
  Branches      569      577    +8     
=======================================
+ Hits         1892     1893    +1     
  Misses          6        6           
  Partials        2        2           
Impacted Files Coverage Δ
src/RangePicker.tsx 100.00% <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 2b5a239...d6e2465. Read the comment docs.

@Kim-Wing-Fung
Copy link
Contributor Author

@zombieJ @afc163

@zombieJ
Copy link
Member

zombieJ commented Jul 6, 2020

Just add param for onCalendarChange instead of new api pls.

@Kim-Wing-Fung
Copy link
Contributor Author

@zombieJ Unfortunately, adding params to onCalendarChange would not allow the desired functionality and would change existing behaviour of onCalendarChange. The onSelect api is added in order to mirror the functionality of onSelect in the PickerPanel component. onCalendarChange is only triggered when a complete action is made i.e. both the start date and end date values have been entered, however the onSelect is intended to trigger when either the start date or end date values have been entered. One example of a use case is being able to disable certain end dates based on the selected start date. Since onCalendarChange only triggers when both start date and end date are entered, it is currently not possible to determine what the selected start date is before an end date has been entered or vice versa. If there are issues with introducing a new api, I am very open to alternative approaches to determine the selected start date or end date individually.

@zombieJ
Copy link
Member

zombieJ commented Jul 7, 2020

onCalendarChange is not onChange. Just check the code under your new added code:

    if (onSelect) {
      if (!isEqual(generateConfig, getValue(mergedValue, 0), startValue)) {
        onSelect(startValue, startStr, 'start');
      }

      if (!isEqual(generateConfig, getValue(mergedValue, 1), endValue)) {
        onSelect(endValue, endStr, 'end');
      }
    }

    if (onCalendarChange) {
      onCalendarChange(values, [startStr, endStr]);
    }

@Kim-Wing-Fung
Copy link
Contributor Author

@zombieJ apologies for my confusion between onCalendarChange and onChange. I will re-examine the onCalendarChange functionality. Thank you for your clarification.

@Kim-Wing-Fung
Copy link
Contributor Author

@zombieJ as you mentioned, I've now updated the onCalendarChange parameters to incorporate additional information. This should not break existing functionality or change any APIs.

@zombieJ
Copy link
Member

zombieJ commented Jul 8, 2020

Thanks for quick response. Could you help to update info to an object like:

info: {


if (onCalendarChange) {
onCalendarChange(values, [startStr, endStr]);
const startChanged = !isEqual(generateConfig, getValue(selectedValue, 0), startValue);
Copy link
Member

Choose a reason for hiding this comment

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

Just use sourceIndex to get the updated input.

@Kim-Wing-Fung
Copy link
Contributor Author

@zombieJ updated info as requested. Please take another look.

@zombieJ zombieJ merged commit 71ad1ba into react-component:master Jul 8, 2020
@zombieJ
Copy link
Member

zombieJ commented Jul 8, 2020

Merged. Could you help to update doc in antd also?

@zombieJ
Copy link
Member

zombieJ commented Jul 8, 2020

+ rc-picker@1.12.0

@Kim-Wing-Fung
Copy link
Contributor Author

@zombieJ Sure, I will do a PR for antd doc soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants