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

Fix "Now" in DateTime parameter not working #3808

Merged
merged 3 commits into from
May 16, 2019

Conversation

gabrieldutra
Copy link
Member

@gabrieldutra gabrieldutra commented May 16, 2019

What type of PR is this? (check all applicable)

  • Bug Fix

Description

I did find the cause for this bug, but with DatePicker methods and our current needs the only way I could think as a fix was to create an "unnecessary" state. Leaving this as WIP so @ranbena and @kravets-levko can join here (also I have to extend the fix to DateTimeRangeInput as a guarantee).

Antd's DatePicker provides the following callbacks:

  • onChange: Triggered whenever you change the value on the dialog
  • onOk: Triggered only when you click "ok"
  • onOpenChange: Triggered when you open/close the dialog

onOpenChange seemed to be the most reliable event to trigger the update for the Query, so I used it by adding a state to track the value.

The cause for the issue: when you click "Now" it invokes onChange -> onOpenChange fast enough that the component doesn't update.
The workaround in this PR: Considering that React guarantees the order for setState I added a new state to use its callback to make sure currentValue would be updated.

BTW, onOk may seem a good solution, but it's not triggered when you click on "Now".

An alternative is to add the "Apply" button for this too, so the update event could be onChange with no problems. The con is that it's not the best to have both "Ok" and "Apply" to set the parameter.

Edit:

  • Updated to use useRef instead.

Related Tickets & Documents

Closes #3803

Mobile & Desktop Screenshots/Recordings (if there are UI changes)

Dashboard
now-dashboard

Query
now-query

@gabrieldutra gabrieldutra self-assigned this May 16, 2019
@kravets-levko
Copy link
Collaborator

@gabrieldutra I analyzed the code a little bit and I found the bug. DateTimeInput uses useState to keep selected value while user changes selection to fire onChange event only when dropdown is closed. It works when user selects value and then presses "Ok" button: onChange -> setCurrentValue -> render -> "Ok" -> onOpenChange -> onChange (uses new value of currentValue) - keep in mind that functional components will "capture" props and state values until next render. So when user uses "Now" button - sequence is a bit different: onChange -> setCurrentValue -> onOpenChange (immediately!) -> onChange (uses captured previous value). So your fix works just because you switched from functional to class component, new state variable can be removed. Also, you can try to use useRef instead of useState - I tried it, it works as well.

@gabrieldutra
Copy link
Member Author

Thanks @kravets-levko 😁

So your fix works just because you switched from functional to class component, new state variable can be removed.

I'd thought so, however this was one of my tests (the values differ):
date-time

Also, you can try to use useRef instead of useState

State seemed more the "React way", but useRef will be the best here indeed. I'll move to it 🙂.

@gabrieldutra gabrieldutra changed the title WIP: Fix "Now" in DateTime parameter not working Fix "Now" in DateTime parameter not working May 16, 2019
@gabrieldutra gabrieldutra marked this pull request as ready for review May 16, 2019 11:52
@gabrieldutra
Copy link
Member Author

@kravets-levko, updated to use useRef. Seems to be working fine and code much less polluted 👌

Copy link
Collaborator

@kravets-levko kravets-levko left a comment

Choose a reason for hiding this comment

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

Very nice fix (even better than I did in my experiments 😅)! And, of course, it works (I tested on query and dashboard page).

@kravets-levko kravets-levko merged commit b263bb7 into master May 16, 2019
@gabrieldutra gabrieldutra deleted the date-time-on-open-change branch May 23, 2019 17:35
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.

"Now" in DateTime parameter isn't working
2 participants