[WIP] Parameter input feedback

[ranbena]

• Feature

Description

Fixes #2721.
Depends on #4049.

1. Shows "Required field" feedback when param input is left empty
2. Disallows query execution with missing params.
3. Added to all parameter input instances - a) Query b) Dashboard c) Widget d) Static widget value.

Here's the whole gang:

Notice that the Number param also checks that input is a number.
Notice dropdown can't be invalid so has no error.

[ranbena]

@arikfr how do you feel about this approach?
https://deploy-preview-4197--redash-preview.netlify.com/

Can also add "Invalid input" error (i.e. NaN in InputNumber)

[gabrieldutra]

Some notes:

• After Introduce inheritance to the Parameter structure #4049 the pendingValue should be normalized as well and empty values will be standardized as null 🚀
• Should we check if the Parameter Value is empty in case there is no pendingValue? (e.g: https://deploy-preview-4197--redash-preview.netlify.com/queries/216 has empty values)

[ranbena]

This line of code is totally temporary.
@gabrieldutra wdyt of this for Parameter.js?

get isEmpty() {
  const value = this.hasPendingValue ? this.pendingValue : this.value;
  return this.isEmptyValue(value);
}

It makes sense to me that param.isEmpty always responds with the currently selected value.

[gabrieldutra]

Currently the most relevant usage of that (or the only one) is to determine the parameters with missing values prior to a query execution, so that would need to be updated (in this case it's necessary to consider the parameter value, not the pending value).

I'm actually "centered positioned" on that. Depends if it makes more sense for the Parameter structure to reflect the "actual Input" or the "Query Execution". For the first one the pendingValue is part of it, for the second one it's just a "state" the parameter has. Considering the existence of this.isEmptyValue(anyValue) it doesn't matter the direction we choose.

BTW, considering "Can also add "Invalid input" error (i.e. NaN in InputNumber)", one idea is to remove the isEmpty (no usage besides deciding if there is an error prior to the query execution) and work on a generalized getErrors with anything that should return an UI error.

[ranbena]

I'm for param.getCurrentInputError() which would return a string ("Required field", "Invalid input", etc) or null. I can add it in this PR 👍

[arikfr]

The indication on the field is the first part, but we also need to revise the big red message as well (either replace or remove).

[ranbena]

The indication on the field is the first part, but we also need to revise the big red message as well (either replace or remove).

Sure, it'll be removed when I publish the PR.

[arikfr]

Sure, it'll be removed when I publish the PR.

I have a feeling removing it will be a big part of the work :)

[ranbena]

Added specific error msg in case query or dashboard init with invalid params.
Notice, these will become edge cases as the UI will now not allow executing an invalid param query.

Query:

Dashboard widget link:

[ranbena]

@arikfr before I release this to be reviewed, here are a few points to be aware of:

• In a query view, newly added parameters immediately show "Required field" error state. I think better ux would be to indicate the added parameter and show the error only after query save. Not an easy implementation.
• Param value errors do not prevent saving the query. Dunno if that's something we should do or not.
• Query error message styling has been left as is. I think it's out of this PR's scope.
• Changing a param type to DateRange causes "missing param" error from server (cause the query itself doesn't include correlated start/end). In a follow-up PR I'll try to make the message actually accurate.

Lmk if any of these are show stoppers.

[ranbena]

Changing a param type to DateRange causes "missing param" error from server (cause the query itself doesn't include correlated start/end). In a follow-up PR I'll try to make the message actually accurate.

Now handled with dedicated param validation check.

When changing param type to DateRange

When changing DateRange param to another type

[ranbena]

Swapped for a backend validation version #4264