Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(dashboard): add ability to change order of variable dropdowns #12821

Merged
merged 1 commit into from
Mar 25, 2019

Conversation

AlirieGray
Copy link
Contributor

@AlirieGray AlirieGray commented Mar 21, 2019

Closes #12512
Closes #12376

This PR allows variables to be moved left and right within the VariablesControlBar in a dashboard. The order of the variables on a given dashboard is stored in the values property of the Variables state in local storage.

2019-03-21 11 08 28

  • CHANGELOG.md updated with a link to the PR (not the Issue)
  • Rebased/mergeable
  • Tests pass

@AlirieGray AlirieGray force-pushed the variables/drag-variables-on-dashboard branch 9 times, most recently from 678f4a3 to f98e1c4 Compare March 22, 2019 20:44
Copy link
Contributor

@chnn chnn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome. I think there's one remaining bug in the reducer code (it was preventing me from loading a dashboard). After that is fixed, LGTM


// Components
import VariableDropdown from 'src/dashboards/components/variablesControlBar/VariableDropdown'
// import VariableDropdown from 'src/dashboards/components/variablesControlBar/VariableDropdown'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// import VariableDropdown from 'src/dashboards/components/variablesControlBar/VariableDropdown'

@@ -81,13 +82,29 @@ export const variablesReducer = (

case 'SET_VARIABLE_VALUES': {
const {contextID, status, values} = action.payload
const originalOrder = draftState.values[contextID].order
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line was preventing me from loading a dashboard that had never been loaded before. In this case there are no values in localStorage, so draftState.values[contextID] doesn't exist, so accessing its order property is a run time error.

I think we can also simplify the logic that maintains the existing order:

        const prevOrder = get(draftState, `values.${contextID}.order`) || []

        if (values) {
          // Maintain the existing sort order, placing new variables at the
          // beginning
          const order = Object.keys(values).sort(
            (a, b) => prevOrder.indexOf(a) - prevOrder.indexOf(b)
          )

          draftState.values[contextID] = {
            status,
            values,
            order,
          }
        } else if (draftState.values[contextID]) {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clever! I like it

@AlirieGray AlirieGray force-pushed the variables/drag-variables-on-dashboard branch 2 times, most recently from 4211f47 to acad0ff Compare March 25, 2019 17:07
@AlirieGray AlirieGray force-pushed the variables/drag-variables-on-dashboard branch from acad0ff to 85e44b2 Compare March 25, 2019 19:40
@AlirieGray AlirieGray merged commit dcd65ee into master Mar 25, 2019
@AlirieGray AlirieGray deleted the variables/drag-variables-on-dashboard branch March 25, 2019 20:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants