Set the first option as current when none is given #99
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
najisawas
reviewed
Mar 31, 2023
| } | ||
| this.state.current = defaultOption | ||
| } else if (!this.state.current && !this.state.includeAll) { | ||
| } else if (!this.state.current) { |
There was a problem hiding this comment.
I'm pretty sure this will break includeAll. For example if includeAll == true then it will get ignored and the dash will default to the newOptions[0], right?
i remember playing with something related to this area in grafana. I would recommend looking into how current is set when theres no allValue. For example if the allValue is "" i believe current is set to null. But if there is an allValue then it's used
|
To be honest I was also quite surprised about the finding but we had a lot
of working dashboards on the prev grafana version. Once onboarded to the v.
9.3.8 they started failing. This fix seems to solve it. Please test it out
too.
fre. 31. mar. 2023 kl. 17.49 skrev Naji Sawas ***@***.***>:
… ***@***.**** commented on this pull request.
------------------------------
In grafana/templates/custom.js
<#99 (comment)>:
> @@ -92,7 +92,7 @@ Custom.prototype._processOptions = function _processOptions() {
throw new SyntaxError("default value not found in options list")
}
this.state.current = defaultOption
- } else if (!this.state.current && !this.state.includeAll) {
+ } else if (!this.state.current) {
I'm pretty sure this will break includeAll. For example if includeAll ==
true then it will get ignored and the dash will default to the
newOptions[0], right?
i remember playing with something related to this area in graphana. I
would recommend looking into how current is set when theres no allValue.
For example if the allValue is "" i believe current is set to null. But
if there is an allValue then it's used
—
Reply to this email directly, view it on GitHub
<#99 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AR6YAWRFQLAHIOXRWHJMFCLW634KDANCNFSM6AAAAAAWOWBT6M>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When starting to use grafana varsion 9.3.8 we have issues when the templates do not have the current value set. This was prev. the case when includeAll was true and not current or defaultvalue was set. In this diff we ensure that current is always set