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

Variables: Add new allowCustomValue flag to MultiVariables #956

Merged
merged 6 commits into from
Nov 12, 2024

Conversation

mdvictor
Copy link
Collaborator

@mdvictor mdvictor commented Nov 6, 2024

We are a feature request to offer the ability to select whether a variable can support custom values or just lock the options list to contain only queried option values. This change is needed in scenes to offer the functionality in dashboards.

Since we currently allow custom values by default, we maintain this default but offer the option to disable the flag in dashboards variable settings.

Related PR

📦 Published PR as canary version: 5.24.0--canary.956.11779891409.0

✨ Test out this PR locally via:

npm install @grafana/scenes-react@5.24.0--canary.956.11779891409.0
npm install @grafana/scenes@5.24.0--canary.956.11779891409.0
# or 
yarn add @grafana/scenes-react@5.24.0--canary.956.11779891409.0
yarn add @grafana/scenes@5.24.0--canary.956.11779891409.0

@mdvictor mdvictor marked this pull request as ready for review November 7, 2024 08:44
@mdvictor mdvictor added minor Increment the minor version when merged release Create a release when this pr is merged labels Nov 8, 2024
Copy link
Contributor

@harisrozajac harisrozajac left a comment

Choose a reason for hiding this comment

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

Tested this by enabling and disabling custom values and it work's as expected, nice work. I just had a couple of nits that you can disregard :)

@@ -40,7 +40,7 @@ export function toSelectableValue<T>(value: T, label?: string): SelectableValue<
}

export function VariableValueSelect({ model }: SceneComponentProps<MultiValueVariable>) {
const { value, text, key, options, includeAll, isReadOnly } = model.useState();
const { value, text, key, options, includeAll, isReadOnly, allowCustomValue } = model.useState();
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a small nit that you can disregard, but I thought it would be nice to just initialize allowCustomValue = true since it's same as allowCustomValue ?? true on L83

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

True, thank you for leaving this! It is nicer indeed! Just updated it.

@@ -101,7 +101,7 @@ export function VariableValueSelect({ model }: SceneComponentProps<MultiValueVar
}

export function VariableValueSelectMulti({ model }: SceneComponentProps<MultiValueVariable>) {
const { value, options, key, maxVisibleValues, noValueOnClear, includeAll, isReadOnly } = model.useState();
const { value, options, key, maxVisibleValues, noValueOnClear, includeAll, isReadOnly, allowCustomValue } = model.useState();
Copy link
Contributor

Choose a reason for hiding this comment

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

same here

mdvictor and others added 2 commits November 11, 2024 12:11
Co-authored-by: Sergej-Vlasov <sergej.s.vlasov@gmail.com>
@mdvictor mdvictor merged commit b8bf1ee into main Nov 12, 2024
5 checks passed
@mdvictor mdvictor deleted the mdvictor/add-variable-option-to-lock-value-list branch November 12, 2024 15:12
@scenes-repo-bot-access-token
Copy link

🚀 PR was released in v5.24.0 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
minor Increment the minor version when merged release Create a release when this pr is merged released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants