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

fix(FE): variables with multiple values are merged into one string #2960

Merged

Conversation

Bergiu
Copy link
Contributor

@Bergiu Bergiu commented Jun 22, 2023

Fixes #2526

@Bergiu Bergiu requested a review from palashgdev as a code owner June 22, 2023 09:18
@welcome
Copy link

welcome bot commented Jun 22, 2023

Welcome to the SigNoz community! Thank you for your first pull request and making this project better. 🤗

@CLAassistant
Copy link

CLAassistant commented Jun 22, 2023

CLA assistant check
All committers have signed the CLA.

@srikanthccv srikanthccv self-requested a review June 22, 2023 09:29
@palashgdev
Copy link
Contributor

@Bergiu can you please check the tsc error though it is breaking

@Bergiu
Copy link
Contributor Author

Bergiu commented Jun 27, 2023

Is the Select component from ant or from rc-select?

It's imported from 'antd' but the error message looks like it's using the Select component from rc-select.

If it's from rc-select the function could be:

	const getSelectValue = (): string | string[] => {
		if (Array.isArray(variableData.selectedValue)) {
			return variableData.selectedValue.map((item) => item.toString());
		}
		return variableData.selectedValue?.toString() || '';
	};

@Bergiu
Copy link
Contributor Author

Bergiu commented Jun 29, 2023

Well, for the two failing checks, I don't know what I need to do.

@srikanthccv
Copy link
Member

You don't need to worry about them.

@Bergiu
Copy link
Contributor Author

Bergiu commented Jul 6, 2023

Is there any problem left or is the issue ready to be merged?

@srikanthccv
Copy link
Member

Sorry we haven't had a chance to review/test this. Will get back to you on this soon.

@srikanthccv
Copy link
Member

I tested this. This seems to have fixed the original issue. I will be testing it for other types, such as queries.

srikanthccv
srikanthccv previously approved these changes Jul 24, 2023
Copy link
Member

@srikanthccv srikanthccv left a comment

Choose a reason for hiding this comment

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

Thank you

@Bergiu
Copy link
Contributor Author

Bergiu commented Jul 25, 2023

Thanks for approving, but the merge is still blocked because of the update to the main branch. Could you merge it directly after the update branch+approve?

@palashgdev
Copy link
Contributor

@Bergiu small changes after LGTM :-D to merge

srikanthccv
srikanthccv previously approved these changes Jul 26, 2023
@srikanthccv
Copy link
Member

@palashgdev please take a look

@palashgdev
Copy link
Contributor

thanks @Bergiu for the PR :-D

@palashgdev palashgdev merged commit aada906 into SigNoz:develop Aug 2, 2023
7 of 9 checks passed
@welcome
Copy link

welcome bot commented Aug 2, 2023

Congrats on merging your first pull request!
minion-party
We here at SigNoz are proud of you! 🥳

@Bergiu Bergiu deleted the issue/2526-multi-value-variables-bug branch August 2, 2023 16:47
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.

Variables with multiple values are merged into one string
4 participants