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

Migration: New datasource #23221

Merged
merged 4 commits into from
Apr 1, 2020
Merged

Migration: New datasource #23221

merged 4 commits into from
Apr 1, 2020

Conversation

tskarhed
Copy link
Contributor

#22862

  • Migrates datasource/new
  • Adds possiblity to use <Icon/> for the icon props on Buttons. Any thoughts about this?

href={`${learnMoreLink.url}?utm_source=grafana_add_ds`}
target="_blank"
rel="noopener"
onClick={onLearnMoreClick}
icon={<Icon name="external-link" />}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Any thoughts about this usage?

Copy link
Member

Choose a reason for hiding this comment

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

can't we just set icon to "external-link"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It originally supported setting it as fa fa-external-link I think. Should maybe remove the fa then, and do some reasonable migration.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll use the fa string for now, and we'll see about that later.

Copy link
Member

@torkelo torkelo left a comment

Choose a reason for hiding this comment

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

Looks ok, some questions. Not sure why we can't pass icon as a string instead , and instead use React element

<input
ref={ref}
export const FilterInput = forwardRef<HTMLInputElement, Props>((props, ref) =>
props.useNewForms ? (
Copy link
Member

Choose a reason for hiding this comment

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

Not sure we need this, we can always use new form styles (i think) as this is only used in the top above lists and not together with other input elements

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. I think the pages using it are fairly simple to migrate too, so we should probably be fine.

href={`${learnMoreLink.url}?utm_source=grafana_add_ds`}
target="_blank"
rel="noopener"
onClick={onLearnMoreClick}
icon={<Icon name="external-link" />}
Copy link
Member

Choose a reason for hiding this comment

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

can't we just set icon to "external-link"

@tskarhed tskarhed merged commit d7d94d1 into master Apr 1, 2020
@tskarhed tskarhed deleted the forms/migration/new-datasource branch April 1, 2020 13:55
@tskarhed tskarhed added this to the 7.0-beta1 milestone Apr 27, 2020
@dprokop dprokop changed the title Forms migration: New datasource Migration: New datasource Apr 28, 2020
@ying-jeanne ying-jeanne added pr/external This PR is from external contributor and removed pr/external This PR is from external contributor labels Apr 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants