Dashboards/variables toolbar #12655

AlirieGray · 2019-03-15T20:58:44Z

This PR adds a new toolbar to the dashboard with dropdowns for all of the variables used in that dashboard. The dropdowns allow the user to select the value that will be used for a given variable everywhere that variable is used on the dashboard they are viewing.

The toolbar can be shown or hidden with a button in the Dashboard Header.

Rebased/mergeable
Tests pass
CHANGELOG.md updated

chnn

Looks awesome. I left a comment about extracting some logic out of components into selector functions that I think would be nice to do. Lemme know if you want any help with that.

chnn · 2019-03-15T21:13:59Z

ui/src/clockface/components/empty_state/EmptyState.tsx

@@ -16,6 +16,7 @@ import {ErrorHandling} from 'src/shared/decorators/errors'

 interface PassedProps {
  children: JSX.Element | JSX.Element[]
+  className?: string


I think most of the other clockface components are using customClass for a prop like this

chnn · 2019-03-15T21:15:19Z

ui/src/dashboards/components/variablesControlBar/VariablesControlBar.tsx

+@ErrorHandling
+class VariablesControlBar extends PureComponent<Props> {
+  render() {
+    // return <div className="variables-control-bar" />


chnn · 2019-03-15T21:18:09Z

ui/src/dashboards/components/variablesControlBar/VariablesControlBar.tsx

+
+    return (
+      <div className="variables-control-bar">
+        {Object.keys(this.variablesForDashboard).map(variableID => {


How about just passing only the id of the variable to the VariableDropdown, and letting the VariableDropdown read its own state from Redux?

chnn · 2019-03-15T21:24:39Z

ui/src/dashboards/components/variablesControlBar/VariablesControlBar.tsx

+    const variableIDs = Object.keys(this.valuesForDashboard)
+    let variablesForDash = {}
+
+    Object.keys(variables).forEach(variableID => {


I think this stuff would belong better in a "selector" utility function. A selector is a function that reads Redux state and transforms/filters it into a nice format for a component to use. Selectors get used in the mstp for a component. That way the component stays focused only on presentation, and the selection logic can be reused across components.

Here is an example of a simple selector and where it gets used.

A lot of people also memoize selectors for performance reasons, but it's probably a detail not worth bothering with at the moment.

chnn · 2019-03-15T21:25:19Z

ui/src/dashboards/components/variablesControlBar/VariablesControlBar.tsx

+            this.valuesForDashboard,
+            `${variableID}.selectedValue`
+          )
+          const name = _.get(


I commented below about selectors... I think this is also a good example of what could be a selector that gets used within the mstp for the VariableDropdown.

chnn · 2019-03-15T21:25:47Z

ui/src/variables/actions/index.ts

@@ -69,6 +74,20 @@ const removeVariable = (id: string): RemoveVariable => ({
  payload: {id},
 })

+interface SelectValue {


heh I just wrote something very similar. Let's merge yours first and then I'll consolidate after merging my PR.

chnn · 2019-03-15T21:26:44Z

ui/src/variables/reducers/index.ts

@@ -78,6 +78,13 @@ export const variablesReducer = (
        return
      }

+      case 'SELECT_VALUE': {
+        const {contextID, id, value} = action.payload
+        draftState.values[contextID].values[id].selectedValue = value


We might want to check that this path exists before trying to set it (e.g. that values[contextID] exists and values[id] exists and so on).

would something like if (!!draftState.values[contextID].values[id].selectedValue) work?

I think that will still throw an error if the values[contextID] part doesn't exist, or if the values[id] part doesn't exist (would see something like Error: can't get selectedValue of null)

How about:

const {contextID, id, selectedValue} = action.payload const valuesExist = !!get( draftState, `values.${contextID}.values.${id}` ) if (!valuesExist) { return } draftState.values[contextID].values[id].selectedValue = selectedValue

since what

chnn · 2019-03-15T21:27:20Z

ui/src/dashboards/components/variablesControlBar/VariableDropdown.tsx

+  variableID: string
+  values: string[] //todo
+  initialSelected: string // todo
+  onSelect: (variableID: string, value: string) => void // todo


oh yeah, I just wanted to check with you that the values should be strings

mavarius

Just a few comments

mavarius · 2019-03-15T21:23:38Z

ui/src/clockface/components/empty_state/EmptyState.tsx


-    const className = `empty-state empty-state--${size}`
+    const classname = className


Since the logic here is starting to get more complex, we can bring in the classnames library and follow the pattern used in other components

is there a good example of this pattern with a customClass property?

IndexListRow.tsx is a good example.

mavarius · 2019-03-15T21:29:55Z

ui/src/dashboards/components/variablesControlBar/VariableDropdown.tsx

+interface Props {
+  name: string
+  variableID: string
+  values: string[] //todo


What are these todo's?

mavarius · 2019-03-15T21:37:03Z

ui/src/dashboards/components/variablesControlBar/VariablesControlBar.tsx

+@ErrorHandling
+class VariablesControlBar extends PureComponent<Props> {
+  render() {
+    // return <div className="variables-control-bar" />


What's this comment?

mavarius · 2019-03-15T21:50:57Z

ui/src/dashboards/components/variablesControlBar/VariablesControlBar.tsx

+
+    return (
+      <div className="variables-control-bar">
+        {Object.keys(this.variablesForDashboard).map(variableID => {


This (and the nested function calls in it) should probably be considered for refactoring. It's not very readable or maintainable in this state.

mavarius · 2019-03-15T22:00:28Z

ui/src/variables/reducers/index.ts

@@ -78,6 +78,13 @@ export const variablesReducer = (
        return
      }

+      case 'SELECT_VALUE': {
+        const {contextID, id, value} = action.payload
+        draftState.values[contextID].values[id].selectedValue = value


This is a pretty long chain, is there any point where one of these 6 items could be undefined?

chnn · 2019-03-18T20:30:12Z

ui/src/variables/selectors/index.tsx

+
+  const variablesIDs = Object.keys(get(values, `${dashboardID}.values`))
+
+  variablesIDs.forEach(variableID => {


I think this might supposed to be:

variablesIDs.forEach(variableID => { const variable = get(variables, `${variableID}.variable`) if (variable) { variablesForDash.push(variable) } })

chnn · 2019-03-18T20:30:50Z

ui/src/dashboards/components/variablesControlBar/VariableDropdown.tsx

+}
+
+interface OwnProps {
+  name: string


This and onSelect could come from Redux too (but it's also fine like this)

I thought of that but I thought a utility was a lot for something we already have peeled off in the parent

there's already a selector that could be useful:

influxdb/ui/src/variables/selectors/index.tsx

Lines 115 to 117 in 243f1ea

export const getVariable = (state: AppState, variableID: string): Variable => {

return get(state, `variables.variables.${variableID}.variable`)

}

then mstp could look something like

const mstp = (state: AppState, props: OwnProps): StateProps => { const {variableID} = props const {name} = getVariable(state, variableID) return {name} }

just as an example for how selectors can be reused.... but this is totally down the weeds and nitpicky, just wanted to share. it's totally cool as is

chnn · 2019-03-18T20:31:06Z

ui/src/dashboards/components/variablesControlBar/VariableDropdown.tsx

+type Props = StateProps & OwnProps
+
+class VariableDropdown extends PureComponent<Props> {
+  constructor(props) {


chnn · 2019-03-18T20:34:33Z

ui/src/variables/selectors/index.tsx

+  state: AppState,
+  variableID: string,
+  contextID: string
+): string => {


Co-Authored-By: Alex Paxton <thealexpaxton@gmail.com>

AlirieGray force-pushed the dashboards/variables-toolbar branch 3 times, most recently from 2c474ce to 0688ce5 Compare March 15, 2019 21:07

chnn reviewed Mar 15, 2019

View reviewed changes

mavarius reviewed Mar 15, 2019

View reviewed changes

AlirieGray force-pushed the dashboards/variables-toolbar branch 8 times, most recently from 43c96b8 to 40ee3e5 Compare March 18, 2019 19:03

chnn approved these changes Mar 18, 2019

View reviewed changes

feat(dashboards): add variable control bar for selecting values

260c612

Co-Authored-By: Alex Paxton <thealexpaxton@gmail.com>

AlirieGray force-pushed the dashboards/variables-toolbar branch from 40ee3e5 to 260c612 Compare March 18, 2019 20:59

AlirieGray merged commit 394de87 into master Mar 18, 2019

AlirieGray deleted the dashboards/variables-toolbar branch March 18, 2019 21:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dashboards/variables toolbar #12655

Dashboards/variables toolbar #12655

AlirieGray commented Mar 15, 2019 •

edited

Loading

chnn left a comment

chnn Mar 15, 2019

chnn Mar 15, 2019

chnn Mar 15, 2019

chnn Mar 15, 2019

chnn Mar 15, 2019

chnn Mar 15, 2019

chnn Mar 15, 2019

AlirieGray Mar 15, 2019

chnn Mar 15, 2019

chnn Mar 15, 2019

AlirieGray Mar 15, 2019

chnn Mar 15, 2019

mavarius left a comment

mavarius Mar 15, 2019

AlirieGray Mar 15, 2019

mavarius Mar 15, 2019

mavarius Mar 15, 2019

AlirieGray Mar 15, 2019

mavarius Mar 15, 2019

mavarius Mar 15, 2019

AlirieGray Mar 15, 2019

mavarius Mar 15, 2019

AlirieGray Mar 15, 2019

chnn Mar 18, 2019

chnn Mar 18, 2019

AlirieGray Mar 18, 2019

chnn Mar 18, 2019

chnn Mar 18, 2019

chnn Mar 18, 2019


		const className = `empty-state empty-state--${size}`
		const classname = className


		const variablesIDs = Object.keys(get(values, `${dashboardID}.values`))

		variablesIDs.forEach(variableID => {

	export const getVariable = (state: AppState, variableID: string): Variable => {
	return get(state, `variables.variables.${variableID}.variable`)
	}

Dashboards/variables toolbar #12655

Dashboards/variables toolbar #12655

Conversation

AlirieGray commented Mar 15, 2019 • edited Loading

chnn left a 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

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

mavarius left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlirieGray commented Mar 15, 2019 •

edited

Loading