Add ResourceSelector component #3
Conversation
andresmgot
requested review from
a team,
iwysiu and
sarahzinger
and removed request for
a team
There was a problem hiding this comment.
Looks great!
Part of me wonders if this stuff should just live in grafana/ui since there is not a lot specific here to a resource or sql or aws. But I'm cool with moving this here for now and if we ever want them in other datasources we could move them out then!
| @@ -2,19 +2,19 @@ | |||
|
|
|||
| exports[`ConnectionConfig should render component 1`] = ` | |||
| <fieldset | |||
| className="css-lq6a48" | |||
There was a problem hiding this comment.
Should we use this opportunity to get rid of the snap file entirely?
There was a problem hiding this comment.
I agree, the test is a bit pointless
|
|
||
| return ( | ||
| <InlineField label={props.label} labelWidth={props.labelWidth} tooltip={props.tooltip}> | ||
| <div data-testid={props['data-testid']} title={props.title}> |
There was a problem hiding this comment.
why do we use title again? Can we just remove it? I think generally we want to avoid title attributes if we can: https://www.a11yproject.com/posts/2013-04-22-title-attributes/
There was a problem hiding this comment.
I am using it to show a message when the input is disabled:
Is there a better way of doing this?
There was a problem hiding this comment.
merging this now, let me know if you have a suggestion for this! (I can open a new pr for that)
Common component for a ResourceSelector:
Epic at #28
Design doc
This has been extracted from the current implementation of the same component in Redshift (not really much to review here).
One thing to notice is that I had to upgrade some @grafana dependencies.