Refresh query when parameters update #3737

gabrieldutra · 2019-04-24T18:35:50Z

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

Feature

Description

This is a work on #2316.

So far, I've added a touch state to Query Parameters and it now refreshes query results whenever a parameter is modified (debounce already added). Next step will be to add an "Apply" button for input fields while refining UX.

Refresh query when parameters are modified
Add "Apply" button for input fields and let auto-refresh for the rest (Dropdowns, Time pickers)
Remove button from Poll for results in parameterized embeds #3752

Related Tickets & Documents

#2316

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

Not yet 😴

arikfr

A lot of the changes are in the query view code, but this functionality need to be applied in all the places where we use parameters (dashboards for example). So most of the changes need to be encapsulated in a single place.

Here's a suggestion --

Feels like the "value updated" should be a property/event of the parameter UI. The single parameter UI will notify the <Parameters> component, which will notify its parent about the updated values. The parent will handle refreshing the data. When the change is applied, the query.parameters already holds the new values (so Query#getQueryResult doesn't need to change).

Each type of parameter will have its own logic of when to apply the change and notify parent: dropdowns and date pickers upon selection, while input fields when clicking on Apply.

Maybe we should wait with introducing debounce until we implement the regular functionality and then decide where debounce belongs in this flow.

gabrieldutra · 2019-04-28T14:35:20Z

Thanks for the comments @arikfr, I've imagined this would be applied to other places where Parameters is used. I thought about doing as you mentioned before, the main reason I went with more logic in the view was thinking about the need of an "Apply All" functionality in the future (e.g: when clicking Execute Query button). I though about 2 ways to make it work:

Having an exposed method in Parameters component that would force update, so I could use refs and when needed just run parametersComponent.applyAll() - however, I'm not so sure how to handle it with both Angular and React coexisting
Lift state up - things end up in the view 😆 (that's why I had changed getQueryResults)

Well, I agree that the second one is not ideal as we would repeat a lot of logic across components, so I'm moving to 1. Gonna make it work first without this functionality, then find a way to handle it. In any case, this is sort of what I had in mind.

Regarding debounce, NP, I'm more using it for testing, later we figure out where it should be.

arikfr · 2019-04-28T19:46:01Z

@gabrieldutra I don't think the component need to expose a method, but rather it should use the same data model object that the parent component will use.

Also, not sure that executing a query should apply the new inputs. It's probably simpler to have the parent components see the old value until it's applied by the input component. This should make implementation simpler and makes sense UX wise.

gabrieldutra · 2019-04-29T15:41:49Z

Also, not sure that executing a query should apply the new inputs. It's probably simpler to have the parent components see the old value until it's applied by the input component. This should make implementation simpler and makes sense UX wise.

I thought there could be a case of pressing, for example, Ctrl+Enter all inputs would be applied. In any case, I'm with: "It's probably simpler to have the parent components see the old value until it's applied by the input component".

For the sake of moving forward, I've added the "Apply Button" and played a little bit with it:

Current implementation

With auto width - it's kind of cool, but there's the issue of line-breaking as you increase the final width

There was an issue with Spinners in Number Inputs and the parameters, so I've removed them. LMK if you prefer a different solution 🙂

@arikfr, @ranbena suggestions for UX/UI here?

ranbena · 2019-04-30T19:30:43Z

@gabrieldutra these are very cool suggestions.

I would rather avoid a button within the input element cause it may cause unforeseen problems.
Should there be a cancel button along with the apply?

My tendency is to do something similar to the UI in param editor:

So perhaps something like this:

That when clicked turns to this:

arikfr · 2019-04-30T19:35:24Z

@ranbena there is no need for edit button, as this is an input field... Having the buttons outside of the control does indeed solve some issues, but is it clear what it will do?

Although I wonder if we should implement this UI:

(including static label for value until edited) 🤔

I'll sleep on it.

@kravets-levko @rauchy wdyt?

ranbena · 2019-05-01T04:34:03Z

@ranbena there is no need for edit button, as this is an input field... Having the buttons outside of the control does indeed solve some issues, but is it clear what it will do?

I meant for this "edit" button to swap the "cog" (should keep it cog tho), and when input.onChange to swoosh the apply/cancel buttons in over it.

arikfr · 2019-05-01T05:01:46Z

I meant for this "edit" button to swap the "cog" (should keep it cog tho), and when input.onChange to swoosh the apply/cancel buttons in over it.

Oh, I see now. Well, the cog only visible for the query editor in edit mode, so not sure if I would bother with moving it or changing it.

My concern that it won't be intuitive what this confirm/cancel button do. But it seems like the simplest solution for now, so maybe worth starting with it and iterating from there.

@gabrieldutra wdyt?

rauchy · 2019-05-01T05:45:26Z

I still find the word "Apply" more descriptive and easy to understand than an icon.

gabrieldutra · 2019-05-02T01:44:03Z

I would rather avoid a button within the input element cause it may cause unforeseen problems.

I did validate this for different resolutions, but I share the same concern.

Should there be a cancel button along with the apply?

If the button will be added in the "same place" as the input (not in a popover) I think it's better not to have a cancel button. Seems simpler without it + I don't think it adds much in this case.

Regarding Static label (or Popover): the idea is interesting, but it adds a new step to the process. I mean, when editing 1 parameter it's fine, but when editing 5 this may get annoying. Also, it wouldn't be possible to use only the keyboard when editing more than 1 parameter.

I agree with @rauchy that the word "Apply" is more intuitive. Perhaps explore it without it being within the input?

arikfr · 2019-05-02T06:53:36Z

My thoughts after reviewing this again and playing with the current version:

I agree that Apply is more intuitive. But placing it outside of the Input will most likely make it less intuitive, so I feel that this is not worth exploring.
Auto width doesn't feel necessary, because the input is large enough as it is even after adding the button.

It seems that the only issue are the spinners for numbers -- I wasn't that attached to them, so don't mind them going away. If we get feedback they were useful, we can explore other options. But for now I think that your current implementation is good 👌

Let's move on ⏩ and get this implemented :)

gabrieldutra · 2019-05-02T22:18:30Z

I'll have to work on a different approach to it anyway, following @ranbena's comment, I've just tested it on Firefox:

Remove the border from the input and put it in a div that wraps both the input and the button should be more reliable and give the same effect. I think I'll leave as is until I finish the core functionality, just in case other ideas come :)

arikfr · 2019-05-05T12:24:17Z

I think I'll leave as is until I finish the core functionality, just in case other ideas come :)

Good call. Let's finish the functionality, add debounce where needed and do the FF fix in a follow up PR so we can release this already. While the bouncing inputs are annoying, it's still better than current behavior.

gabrieldutra · 2019-05-06T01:12:43Z

I've updated this to include the Apply Button in the other places. @arikfr, is anything missing for this to be done in terms of functionality? If not, this can be open for review 🙂

The best place to put debounce seemed to be the Parameters component in the onChange method. Ideally this should debounce the parameter's setValue as well, but this will be easier when we migrate the Parameters component (by using states and so).

gabrieldutra · 2019-05-10T16:30:34Z

@ranbena, I've updated the styling after considering your comments :). What did you base right: 4px; in?

Should it be 5px instead to fit top & bottom of Apply Button's position?

When text is long and starting to edit, the edited area is suddenly out of bounds, so user has to stop and scroll it into view to keep editing..

This is sort of an edge case if you consider that when you go on typing it automatically goes to the caret position, I think if there is an easy solution we should do. I'll see what I find for this.

Since all input param types now apply only on selection / apply click - no reason to debounce at all, right?

I think the idea is to have some time to apply more parameters without executing the query multiple times (although the 1 sec I let for tests seems not to be enough ^^)

ranbena · 2019-05-10T16:39:38Z

@ranbena, I've updated the styling after considering your comments :). What did you base right: 4px; in?

Looking good. 4px comes from the button's distance from top and bottom. For visual consistency it should be 4px from all sides.

Should it be 5px instead to fit top & bottom of Apply Button's position?

With absolute positioning it should be top: 5px; right: 5px 👍

When text is long and starting to edit, the edited area is suddenly out of bounds, so user has to stop and scroll it into view to keep editing..

This is sort of an edge case if you consider that when you go on typing it automatically goes to the caret position, I think if there is an easy solution we should do. I'll see what I find for this.

👍

Since all input param types now apply only on selection / apply click - no reason to debounce at all, right?

I think the idea is to have some time to apply more parameters without executing the query multiple times (although the 1 sec I let for tests seems not to be enough ^^)

Why is it needed though? Doesn't the previous execution get cancelled on re-execution?
The debounce has a bad UX effect in which you click "apply" and nothing happens for a sec. It's annoying and we should avoid it.

ranbena · 2019-05-10T17:14:23Z

This is sort of an edge case if you consider that when you go on typing it automatically goes to the caret position, I think if there is an easy solution we should do. I'll see what I find for this.

@gabrieldutra just noticed that if you keep typing, Chrome will automatically scroll to caret. So can leave as is.

gabrieldutra · 2019-05-10T17:33:27Z

Why is it needed though? Doesn't the previous execution get cancelled on re-execution?
The debounce has a bad UX effect in which you click "apply" and nothing happens for a sec. It's annoying and we should avoid it.

Totally agreed, I'm not really sure if the previous get cancelled, for MySQL iirc it doesn't work ^^. I guess in such cases it would queue the query executions (which can be an issue for slow queries). For the sake of testing I've removed debounce:

(BTW, interesting way to test slow queries ^^)

It seems that: - when there's one worker available, the triggered execution goes directly to it; - when the execution is queued (not running yet), it is replaced by the new one(?)

ranbena · 2019-05-10T17:49:13Z

Totally agreed, I'm not really sure if the previous get cancelled, for MySQL iirc it doesn't work ^^. I guess in such cases it would queue the query executions (which can be an issue for slow queries). For the sake of testing I've removed debounce:

Well I can tell you the user experience now is soooo much better :)

(BTW, interesting way to test slow queries ^^)

Nice!

It seems that: - when there's one worker available, the triggered execution goes directly to it; - when the execution is queued (not running yet), it is replaced by the new one(?)

@arikfr how does it work?

arikfr · 2019-05-12T11:13:42Z

It seems that: - when there's one worker available, the triggered execution goes directly to it; - when the execution is queued (not running yet), it is replaced by the new one(?)

We deduplicate executions of the same query multiple times, i.e. if the query already executes and you execute it again, it will "join" the previous execution. So in the above case, because you didn't change the values, it was all one execution.

Indeed the idea here was to avoid multiple executions when you want to update several parameters. But thinking about it, I see that debounce is not enough -- what needs to happen is more complex: if you start editing another parameter, we should wait with execution.

Let's roll out without debounce, and improve this based on feedback from users and our experience using this.

arikfr · 2019-05-12T11:14:53Z

Is this ready in terms of implementation?

ranbena · 2019-05-12T11:51:10Z

A sidenote on debounce and UX:
The problem is not the debounce itself, but the delay in "responsiveness". There must be a visual indication that the "apply" click has been registered and acted upon - even if in actuality it's idle.

Similar to the dashboard save debounce implementation.

gabrieldutra · 2019-05-12T23:07:15Z

Is this ready in terms of implementation?

Considering the last comments, yes. I do have a concern about slow queries, but I think it's safe to go through "Let's roll out without debounce, and improve this based on feedback from users and our experience using this.".

There must be a visual indication that the "apply" click has been registered and acted upon - even if in actuality it's idle.

This is sort of why I was keeping with 1 sec of debounce only, but definitively better without debounce or with a visual indication.

ranbena · 2019-05-12T23:20:48Z

client/app/components/ParameterValueInput.jsx

-        onChange={onSelect}
-      />
+      <div
+        className={classNames('parameter-input-number', { 'parameter-input-number--apply-button': showApplyButton })}


When it comes to element state, I tend to favor describing the state rather than it's impact (cause that usually changes as app progresses) so instead of classname parameter-input-number--apply-button I'd go with input-dirty. And if it's a bool, I'd even ditch classnames and do a data-dirty="true" attribute on the element.

For your consideration 👨‍💻

Oh, this is just to have the apply button on/off 🤦‍♂️

Hmm so I would have these as two separate attributes actually.

Where does $ctrl.applyButton determined? When is it false?

Found it - hardcoded to true. Then perhaps this prop can be removed and code simplified?

When is it false?

ParameterMappingInput 🙂 (e.g: in the Add Widget Dialog). Also I thought it could be interesting to support it without the Apply Button.

Regarding the css, yes, it's to turn on/off the appearing of the apply button on the input. I tried to follow BEM naming convention on that, but yes, I can use a data attribute instead. It will actually look better than this classNames syntax.

BTW, it was Cypress that made me realize that the Apply Button would affect the Add Widget Dialog 😁

I tried to follow BEM naming convention on that

Maybe we should have a conversation on whether we want to keep using BEM.

Updated 👍

Maybe we should have a conversation on whether we want to keep using BEM.

This was actually the only case where I was applying BEM (modifiers like --apply-button). But I preferred as @ranbena suggested data-dirty. I think BEM is sort of deprecated for Redash already, just need to update the docs if that's really the case.

ranbena · 2019-05-12T23:38:59Z

Another sidenote - the reason I prefer an icon to textual label in this case ("apply") is that some day we'll incorporate i18n, and this won't do 🔮

arikfr · 2019-05-13T05:58:56Z

Another sidenote - the reason I prefer an icon to textual label in this case ("apply") is that some day we'll incorporate i18n, and this won't do 🔮

🤔 fair point. Let's keep it as is for now with Apply and revisit in the future. I feel like the whole UI for parameters is due for deeper UX work. One option is to have it in a non editable form when presenting and have a proper UI for editing. Here's an example from Intercom:

And when editing:

About an alternative to debounce, we could have a dropdown next to the Apply.

~~Not sure it's appropriate considering the limited space we have for it.~~ We can't do it with current UI, because it will work only with <Input>s but not dropdowns or dates. But this is something we can do once we switch to rendering a dedicated UI for editing.

arikfr · 2019-05-13T15:33:41Z

Date Range parameters don't seem to update on dashboards. See example here: https://deploy-preview-3737--redash-preview.netlify.com/dashboard/legacy-parameters-dashboard?p_dateparam=2019-01-21&p_AlbumId=2001-01-20&p_time_frame.start=2019-01-03&p_time_frame.end=2019-01-11&p_param1=title1&p_date%20range.start=2019-05-24&p_date%20range.end=2019-05-27.

gabrieldutra · 2019-05-13T19:07:14Z

Date Range parameters don't seem to update on dashboards

Actually none of the parameters are updating in there, it seems that debounce had an extra job for the order of functions. I'll dig into it and try to come up with sth more reliable for this part.

gabrieldutra · 2019-05-13T21:23:30Z

client/app/components/ParameterValueInput.jsx

    },
    controller($scope) {
      this.setValue = (value) => {
        this.param.setValue(value);
-        $scope.$applyAsync();
+        $scope.$apply();


@arikfr changing $applyAsync to $apply here did the trick for the parameters not working on dashboards. What happens is that dashboards depends on Url Parameters to be updated (it's a $watch that does it) and the refresh was being triggered before.

Luckily soon we will get rid of all these crap 😎

arikfr · 2019-05-15T05:57:20Z

🤩

* Add touch state to parameters and autoupdate query * Use values change event instead of $watch * Remove getQueryResultDebounced * Add Apply button * Remove Input Number spinners for Parameters * Make Apply Button optional * Update share_embed_spec * Change debounce to the Parameters component * Remove unnecessary click on Execute query * Add apply button to the remaining places * Update dashboard_spec * Use onKeyUp for InputNumber * Simplify onParametersValuesChanged * Update DateTime onChange function * Don't apply when modifier key is pressed * Remove refresh Button from Parameters * Update apply button styling * Update apply right distance * Remove debounce for testing * Use data-dirty instead of classNames for styling * Make sure $apply runs before calling onChange

Add touch state to parameters and autoupdate query

bd50110

arikfr reviewed Apr 24, 2019

View reviewed changes

Use values change event instead of $watch

5c2bc12

Remove getQueryResultDebounced

e16bc91

gabrieldutra added 2 commits April 29, 2019 12:02

Add Apply button

dd730e5

Remove Input Number spinners for Parameters

134334e

Make Apply Button optional

d800e39

arikfr mentioned this pull request May 2, 2019

Poll for results in parameterized embeds #3752

Merged

1 task

Merge branch 'master' into parameters-apply-button

5955360

gabrieldutra added 8 commits May 5, 2019 17:43

Update share_embed_spec

043cf67

Change debounce to the Parameters component

523da04

Merge branch 'master' into parameters-apply-button

3cf9274

Remove unnecessary click on Execute query

c43e19b

Add apply button to the remaining places

daa1f29

Update dashboard_spec

bf123bd

Use onKeyUp for InputNumber

1ca6457

Simplify onParametersValuesChanged

61d4fb4

gabrieldutra added 2 commits May 10, 2019 13:48

Update apply right distance

4c6f7f5

Remove debounce for testing

bc0f418

ranbena reviewed May 12, 2019

View reviewed changes

Use data-dirty instead of classNames for styling

a55edb8

Make sure $apply runs before calling onChange

96624d4

gabrieldutra commented May 13, 2019

View reviewed changes

arikfr merged commit c76955b into master May 15, 2019

arikfr deleted the parameters-apply-button branch May 15, 2019 05:57

arikfr mentioned this pull request May 15, 2019

Esc key should cancel parameter edits #3802

Open

gabrieldutra mentioned this pull request May 17, 2019

Cypress tests for query parameters #3810

Merged

9 tasks

weekly-digest bot mentioned this pull request May 20, 2019

Weekly Digest (13 May, 2019 - 20 May, 2019) #3814

Closed

arikfr mentioned this pull request May 27, 2019

Refresh query when parameters update #2316

Closed

gabrieldutra mentioned this pull request May 29, 2019

Cancel previous execution on parameters value update #3850

Closed

1 task

gabrieldutra mentioned this pull request Jun 17, 2019

Parameter “Apply Changes” button #3907

Merged

1 task

gabrieldutra mentioned this pull request Jun 28, 2019

Add Dynamic Values to Date and Date Range Parameters #3904

Merged

1 task

Refresh query when parameters update #3737

Refresh query when parameters update #3737

Conversation

gabrieldutra commented Apr 24, 2019 • edited Loading

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

Description

Related Tickets & Documents

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

arikfr left a comment

Choose a reason for hiding this comment

gabrieldutra commented Apr 28, 2019

arikfr commented Apr 28, 2019

gabrieldutra commented Apr 29, 2019 • edited Loading

ranbena commented Apr 30, 2019

arikfr commented Apr 30, 2019

ranbena commented May 1, 2019

arikfr commented May 1, 2019

rauchy commented May 1, 2019

gabrieldutra commented May 2, 2019

arikfr commented May 2, 2019

gabrieldutra commented May 2, 2019

arikfr commented May 5, 2019

gabrieldutra commented May 6, 2019

gabrieldutra commented May 10, 2019

ranbena commented May 10, 2019

ranbena commented May 10, 2019

gabrieldutra commented May 10, 2019

ranbena commented May 10, 2019

arikfr commented May 12, 2019

arikfr commented May 12, 2019

ranbena commented May 12, 2019

gabrieldutra commented May 12, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ranbena commented May 12, 2019

arikfr commented May 13, 2019

arikfr commented May 13, 2019

gabrieldutra commented May 13, 2019

gabrieldutra May 13, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arikfr commented May 15, 2019

gabrieldutra commented Apr 24, 2019 •

edited

Loading

gabrieldutra commented Apr 29, 2019 •

edited

Loading

gabrieldutra May 13, 2019 •

edited

Loading