-
Notifications
You must be signed in to change notification settings - Fork 1
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
SQL/ResourceSelector: Allow to set resources #12
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm 👍
Just a comment/question about import order 🙂
import { render, screen } from '@testing-library/react'; | ||
import { ResourceSelector, ResourceSelectorProps } from './ResourceSelector'; | ||
import React from 'react'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm used to having React as first, any reason why you moved it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm now using Typescript Hero to automatically add, order and remove imports. Since it's automatic, it uses the import path to order them (alphabetically). It also differentiate between packages and relative imports.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see!
I would have thought it would put default before named imports, but I don't see anything in the eslint or prettier config for it so good to go 👍
import { SelectableValue } from '@grafana/data'; | ||
import { InlineField, Select } from '@grafana/ui'; | ||
import { isEqual } from 'lodash'; | ||
import React, { useEffect, useMemo, useState } from 'react'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
idem import order
I have a case now that I need to get the list of resources in the parent component so to avoid duplicity, I made it optional to fetch resources in this component.