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

Templating: Migrates some variable types from Angular to React/Redux #22434

Merged
merged 239 commits into from
Mar 10, 2020

Conversation

mckn
Copy link
Contributor

@mckn mckn commented Feb 26, 2020

What this PR does / why we need it:
Fourth iteration trying to move template variables to Redux. This iteration takes the React/Redux parts from the third iteration #21873 and does (almost) no integration with Angular components. Everything is behind the feature toggle called newVariables and will be a completely new property in the dashboard.json => variables.list.

Misc

  • fix tests
  • fix strict null errrors
  • write tests
  • cleanup/refactor code
  • update e2e tests
  • documentation
  • celebrate 🎉

Known issues so far:

  • focus is not following highlighted picker option when navigating with keys (same on play.grafana.com)

When we have nothing else to do:

  • Simplify state handling
  • Create a tag component to handle the picker tags
  • Change so the VariableTag and VariableOption uses text/value: string[] instead of string | string[]
  • Change so thunk signatures take uuid as parameter instead of variable to avoid stupid bugs
  • Write a class that walks the variable tree and create an execution order.

Upcoming PRs

  • Refactor Picker/Picker actions and state into something that can be reused between custom/query/datasource variable types
  • Introduce custom variable type
  • Introduce textbox variable type
  • Introduce constant variable type

Which issue(s) this PR fixes:
Closes #19896

Special notes for your reviewer:
Not covered in this PR

Copy link
Member

@torkelo torkelo left a comment

Choose a reason for hiding this comment

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

Ok 99/99 files viewed!

Amazing work ❤️

Was not much that I could find that looked off or I could not understand the reasons for.

Will do more testing but since it's behind a feature toggle I think we can do that after merge as well.

@hugohaggmark
Copy link
Contributor

Ok 99/99 files viewed!

Amazing work ❤️

Was not much that I could find that looked off or I could not understand the reasons for.

Will do more testing but since it's behind a feature toggle I think we can do that after merge as well.

Thanks for all the great feedback @torkelo and @dprokop and it feels great to have come this far finally!

@mckn mckn merged commit cc813d7 into master Mar 10, 2020
Frontend Platform Backlog automation moved this from In Review to Done Mar 10, 2020
@mckn mckn deleted the hugoh/newVariables branch March 10, 2020 07:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dashboard: Migrate Templating to React/Redux
4 participants