feat(ui): Template variables can now select their source database #5367
Conversation
hoorayimhelping
force-pushed
the
bs_source_databases
branch
2 times, most recently
from
86214ee to
d9c3610
Compare
hoorayimhelping
requested review from
goller,
desa,
glinton and
121watts
and removed request for
goller
| selectedSource, | ||
| } = this.state | ||
|
|
||
| nextTemplate.sourceID = '0' |
There was a problem hiding this comment.
I'm assuming '0' is a source ID that can never exist? Perhaps document this ID's purpose with a variable?
There was a problem hiding this comment.
'0' is the default / dynamic database - so this clearly needs a comment since your assumption is WRONG 😆
| // When we change the source database here and below, we want to reset the template complete | ||
| private handleSelectDynamicSource = () => { | ||
| this.setState({ | ||
| isDynamicSourceSelected: true, |
There was a problem hiding this comment.
Why not a type here instead of a boolean?
Something like type SelectedSourceType = 'static' | 'dynamic'
Then, this.setState({sourceType: 'dynamic'}) etc. Just a style suggestion. Functionally it will be the same. Don't particularly care for booleans. Limits us to only two cases.
And then the code would read so nice like:
if (sourceType === 'dynamic') {
... do dynamic stuff here
}
There was a problem hiding this comment.
We talked about this offline a little bit - I'm going to hold off on doing this right now.
I agree with the assessment 100% The issue is that this pattern is used elsewhere and even though it's a little wonky, I think having consistent wonk for a short while is less confusing than having some wonk and some Done Right™ implementations.
We're planning on having a follow up 1.8.1 release, this might be worth thinking about for that.
go.mod
Outdated
| @@ -29,6 +29,7 @@ require ( | |||
| github.com/influxdata/usage-client v0.0.0-20160829180054-6d3895376368 | |||
| github.com/jessevdk/go-flags v1.4.0 | |||
| github.com/jonboulle/clockwork v0.1.0 // indirect | |||
| github.com/kevinburke/go-bindata v3.16.0+incompatible // indirect | |||
There was a problem hiding this comment.
we probably should just leave this in, as they get injected every time we run make, but it's not used by chronograf outside the makefile. to avoid churn, remove these go.* changes either manually or with a go mod tidy.
| Values: vals, | ||
| Type: t.Type, | ||
| Label: t.Label, | ||
| SourceID: int64(t.SourceID), |
There was a problem hiding this comment.
for uniformity, set this to int(t.SourceID)
hoorayimhelping
force-pushed
the
bs_source_databases
branch
from
d9c3610 to
6afdc59
Compare
hoorayimhelping
force-pushed
the
bs_source_databases
branch
from
6afdc59 to
089de7b
Compare
|
needs docs. @noramullen1 |
Closes #5254
Template variables can now select and use a source database other than the default dynamic one