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

Api query editor #84

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Api query editor #84

wants to merge 2 commits into from

Conversation

scottlepp
Copy link
Contributor

@scottlepp scottlepp commented Nov 15, 2023

component for rendering rest api inputs

  • currently supports: text input, select, async select inputs
  • will evolve to support more inputs, including custom react components

@scottlepp scottlepp requested review from cletter7 and a team November 15, 2023 21:27
<div>
{api.inputs.map((iv: InputValue, i) => {
const val = value(query, i, iv.defaultValue);
return <div className="gf-form" key={i}>
Copy link
Contributor

Choose a reason for hiding this comment

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

From what I know gf-form classes is legacy. Could we use InlineField component instead?

onChange={(selected: SelectableValue<string>, _: ActionMeta) => onChangeSelect(selected, i)}
/>
}
{!iv.choices && !iv.lookup &&
Copy link
Contributor

@cletter7 cletter7 Nov 16, 2023

Choose a reason for hiding this comment

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

Seems like lookup, choices etc. are mutually exclusive and I'd suggest to rethink the InputValue shape. I'd suggest to have something like type field in it which can have a limited number of options like select , asyncselect, input, etc. And I think value / options can be different depending on the type.

The reason I am suggesting this is I think in this way it will be more future proof. Maybe in the future we would want to also render the options as a radio buttons, etc. And for such cases I think we should create API in a way that it's easily extendible. It might be hard to change the API when plugins start using it.

@@ -21,3 +21,4 @@ export { InlineSwitch, CertificationKey } from '@grafana/ui';
export * from './QueryEditor';
export * from './ConfigEditor';
export { CustomHeadersSettings } from './CustomHeadersSettings/CustomHeadersSettings';
export { ApiQueryEditor } from './ApiQueryEditor/ApiQueryEditor';
Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably also export the types so that we can consume them in plugins.

Comment on lines +68 to +70
<InlineFormLabel tooltip={iv.description}>
{pretty(iv.name)}
</InlineFormLabel>
Copy link
Contributor

@cletter7 cletter7 Nov 30, 2023

Choose a reason for hiding this comment

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

I think it's better to provide the label separately than to try to prettify inner name of the field, they might not always match.

Example ("Eventtype" might be ok for id, but the label should be "Event type" I think):
image

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.

None yet

3 participants