Fix: support date ranges for parameterized embeds#3681
Conversation
| const removePrefix = ([k, v]) => [k.slice(2), v]; | ||
| const toObject = (obj, [k, v]) => Object.assign(obj, { [k]: v }); | ||
|
|
||
| export default () => Object.entries(parse(location.search)) |
There was a problem hiding this comment.
Since this parsing function is really specific to redash query parameters, I would:
export default parseLocationSearch = () => Object.entries(parse(location.search));
export parseRedashQueryParameters = () => parseLocationSearch()
.filter(onlyParameters)
.map(removePrefix)
.reduce(toObject, {});Wdyt?
There was a problem hiding this comment.
I think that most of the (non-lib) code in the project is Redash specific but we don't explicitly add "Redash" to function and class names, so I tend to disagree.
There was a problem hiding this comment.
While I agree that prefixing stuff with "Redash" in our codebase doesn't make sense, @ranbena has a point that this is not some generic query string parsing function but a specific one for parameters. It makes sense to name it accordingly, like "parametersFromQueryString" or something along this lines.
arikfr
left a comment
arikfr
left a comment
There was a problem hiding this comment.
This is already merged, but it made sense to comment about this in the context of this PR.
| "pivottable": "^2.15.0", | ||
| "plotly.js": "1.41.3", | ||
| "prop-types": "^15.6.1", | ||
| "qs": "^6.7.0", |
There was a problem hiding this comment.
You had to commit an updated package-lock.json file as well.
There was a problem hiding this comment.
Hmm... I wonder how that slipped. Should I do this in a new PR?
| @@ -0,0 +1,11 @@ | |||
| import qs from 'qs'; | |||
There was a problem hiding this comment.
The file name doesn't follow the convention of filename = name of default export.
There was a problem hiding this comment.
I was unaware of that convention. This will be handled in #3687
| const removePrefix = ([k, v]) => [k.slice(2), v]; | ||
| const toObject = (obj, [k, v]) => Object.assign(obj, { [k]: v }); | ||
|
|
||
| export default () => Object.entries(parse(location.search)) |
There was a problem hiding this comment.
While I agree that prefixing stuff with "Redash" in our codebase doesn't make sense, @ranbena has a point that this is not some generic query string parsing function but a specific one for parameters. It makes sense to name it accordingly, like "parametersFromQueryString" or something along this lines.
A query is a model in Redash, a query can have multiple parameters, a query-string is a part of the url, parameters can be represented inside the query-string, sometimes the query-string is represented in our code as 'query', which has parameters. THIS IS NUTS. |
* support date ranges for parameterized embeds * add qs * remove hideous implementation and use qs * get rid of apiKey querystring parameter and introduce query-string module
3 participants
What type of PR is this? (check all applicable)
Description
Parsing of date ranges was unhandled in #3495, and is now fixed here, though I guess we should really toss this piece of code and consolidate.
Related Tickets & Documents
Reported by @HenroRitchie here, thanks!
Mobile & Desktop Screenshots/Recordings (if there are UI changes)