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

Issue 510 fix clearing date callback #511

Merged

Conversation

Projects
None yet
2 participants
@derekclee
Copy link
Contributor

commented Apr 3, 2019

Fixes a problem with <DatePickerSingle /> where the callback setProps was not called when a date was cleared.

if (setProps && date !== null) {
setProps({date: date.format('YYYY-MM-DD')});
if (setProps) {
setProps({date: date ? date.format('YYYY-MM-DD') : null });

This comment has been minimized.

Copy link
@alexcjohnson

alexcjohnson Apr 4, 2019

Contributor

This looks good to me. I'm a bit concerned about backward compatibility though, as it's been this way for as long as these components have existed and users may depend on null never arriving in their callbacks. @chriddyp thoughts? Do we need to make this behavior opt-in?

This comment has been minimized.

Copy link
@alexcjohnson

alexcjohnson Apr 5, 2019

Contributor

OK, we discussed this internally, the consensus is we're happy to call the old behavior a bug, this PR a fix, so no need for opt-in 🎉

@alexcjohnson

This comment has been minimized.

Copy link
Contributor

commented Apr 4, 2019

Thanks @derekclee! Really appreciate seeing the PR come with tests already in place 🏆 Aside from the question about backward compatibility that I'm not immediately sure how I feel about (if we were starting from scratch I'd much prefer to keep it simple like you have here), the only issue I see is linting - we use prettier, you can run it with npm run format to automagically fix the issues.

derekclee added some commits Apr 3, 2019

Updated `<DatePickerSingle />` to correctly handle clearing.
  - When clearing a date, `date` is set to `null` but `setProps`
    was not called.
  - Made sure to always call `setProps` on any date change.

@derekclee derekclee force-pushed the derekclee:issue-510-fix-clearing-date-callback branch from b745c51 to b29653d Apr 4, 2019

@derekclee

This comment has been minimized.

Copy link
Contributor Author

commented Apr 4, 2019

@alexcjohnson I ran prettier on the files and fixed up the commit.

Our use case of the clearing is that we initially have a blank date filter on a visualization. The date may not always be present in the data thus having the filter optional is important. A user is able to filter by a date. If the date was not clearable, the user would have to do a full refresh of the application to get it back to the original state. I can add an opt in prop if needed.

On an unrelated note, I originally tried to use the DatePickerRange but that would no accept null starting dates. The fix for that is a lot harder as the error seems to be in <DayPicker /> from https://github.com/airbnb/react-dates and this library is using an older version.

@alexcjohnson alexcjohnson referenced this pull request Apr 4, 2019

Merged

Upgrade to React 16 #508

@alexcjohnson
Copy link
Contributor

left a comment

💃

@@ -1076,6 +1076,8 @@ def test_tabs_with_children_undefined(self):

self.startServer(app=app)

self.wait_for_element_by_css_selector('#tabs-content')

This comment has been minimized.

Copy link
@alexcjohnson

alexcjohnson Apr 5, 2019

Contributor

There was a percy diff - the image produced in this PR prior to this commit was actually the right one, dunno how the incorrect one got approved... but to make that more robust I added this wait for the correct element to be on the page.

@alexcjohnson alexcjohnson merged commit 6c9cb71 into plotly:master Apr 5, 2019

4 checks passed

ci/circleci: python-2.7 Your tests passed on CircleCI!
Details
ci/circleci: python-3.6 Your tests passed on CircleCI!
Details
ci/circleci: python-3.7 Your tests passed on CircleCI!
Details
percy/dash-core-components Visual review approved by johnson.alex.c@gmail.com
Details
@derekclee

This comment has been minimized.

Copy link
Contributor Author

commented Apr 5, 2019

Thanks @alexcjohnson !

alexcjohnson added a commit that referenced this pull request Apr 6, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.