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: Fixes so texts show in picker not the values #27002

Merged

Conversation

hugohaggmark
Copy link
Contributor

What this PR does / why we need it:
The templating system has gotten less complex when we migrated it to React but there is still at least one big complexity left that still causes us some issues. The complexity I'm talking about is how we for some VariableTypes have support for both single value and multi value. This causes a lot of if statement in our code that tries to handle this and props that can be both string and string[].

This PR does not attempt to solve this complexity, but I think it pushes our code onto a better path. With this PR the current prop will always keep the same type as defined in alignCurrentWithMulti below:

export const alignCurrentWithMulti = (current: VariableOption, value: boolean): VariableOption => {

This means that OptionsPicker is the only component/class/object/function that transforms the current.text to the correct value which seems a lot clearer to me.

const getLinkText = (variable: VariableWithOptions) => {

I also did some refactoring of the tests because they weren't using "valid" data.

Which issue(s) this PR fixes:
Fixes #25711
Fixes #25959

Special notes for your reviewer:
There are probably other cases that are messed up with this, but I couldn't find any :)

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.

Looks good, tested this with an already saved dashboard with many multi-variables and saved in specific multi selection state to make sure there was no problem loading an old dashboard

@hugohaggmark
Copy link
Contributor Author

Looks good, tested this with an already saved dashboard with many multi-variables and saved in specific multi selection state to make sure there was no problem loading an old dashboard

Thank you @torkelo, before we merge I just want to discuss this change with @mckn because I might miss some of his insights on this.

Copy link
Contributor

@mckn mckn left a comment

Choose a reason for hiding this comment

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

This looks great! It becomes a lot simpler 🙌

awesome

@hugohaggmark
Copy link
Contributor Author

This looks great! It becomes a lot simpler 🙌

awesome

Thank you @mckn

@hugohaggmark hugohaggmark merged commit bd75adf into master Aug 17, 2020
@hugohaggmark hugohaggmark deleted the hugoh/bug-multivalue-rendering-value-instead-of-text branch August 17, 2020 05:04
wbrowne pushed a commit that referenced this pull request Aug 20, 2020
@wbrowne wbrowne mentioned this pull request Aug 20, 2020
wbrowne pushed a commit that referenced this pull request Aug 20, 2020
@aknuds1 aknuds1 mentioned this pull request Aug 20, 2020
@torkelo
Copy link
Member

torkelo commented Aug 22, 2020

This caused a very critical bug, causing the All variable not to work (both for normal cases and repeated panels).

#27119

For repeated panels is due to current.text being an array so a comparison with 'All' no longer works:
https://github.com/grafana/grafana/blob/master/public/app/features/dashboard/state/DashboardModel.ts#L654

That explains why repeat no longer works.

Not sure yet why All is not working in a normal query (not rendering to all the values) but probably a similar issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants