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

Adding datasource picker and datalinks #17

Merged
merged 10 commits into from
Jan 5, 2021
Merged

Conversation

srclosson
Copy link
Member

The datasource picker and datalinks are used in Splunk, but decoupled so the control can be reusable

image

@srclosson srclosson self-assigned this Dec 10, 2020
@srclosson srclosson requested a review from a team December 10, 2020 20:29
@ryantxu
Copy link
Member

ryantxu commented Dec 10, 2020

still super nervous about this repo 😬

Lets make sure it has a plan for how to stay in sync with core

@kminehart
Copy link
Contributor

@ryantxu Don't be nervous! :D

@srclosson
Copy link
Member Author

We should actually come up with a plan for this. Perhaps even print the plan in the README?

I can talk to Luke and see if it's possible to set something up.

@srclosson srclosson marked this pull request as draft December 10, 2020 21:41
@srclosson srclosson marked this pull request as ready for review December 11, 2020 01:19
@vickyyyyyyy
Copy link
Member

still super nervous about this repo 😬

Lets make sure it has a plan for how to stay in sync with core

We spoke to Alex and Luke and they suggested we have a regular (weekly or bi-weekly) sync space to gather all FE Enterprise and OSS things so everyone is aware of what's going on 😄 This way we can also discuss if any components we come up with can also be helpful in the core library.

For more context, they agreed that while we do have Enterprise-specific UIs, it would be great to incorporate all the work into the design system and also a single UI library.

Copy link
Member

@vickyyyyyyy vickyyyyyyy left a comment

Choose a reason for hiding this comment

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

LGTM! Good to see many tests being added! 🔥 🔥 🔥

Just left a few comments and nit suggestions 😄

I noticed the expect assertions are inside of the act wrapper - the act API is for wrapping interactions with your applications, like event firing, so the expect assertions are normally left outside and not included within the wrapper.

I think the awaits within the act wrappers can also be removed and another nit: the onChange handlers can be passed in jest.fn() rather than () => {} if we don't care about them

EDIT: Forgot to ask, can we add some stories for the new components so we have examples of their usages? 🙏

Comment on lines 4 to 6
// import { Button } from '@grafana/ui';
// import { DataLink } from './DataLink';
// import { act } from 'react-dom/test-utils';
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// import { Button } from '@grafana/ui';
// import { DataLink } from './DataLink';
// import { act } from 'react-dom/test-utils';
// import { Button } from '@grafana/ui';
// import { DataLink } from './DataLink';
// import { act } from 'react-dom/test-utils';

src/components/DataLinks/DataLinks.test.tsx Outdated Show resolved Hide resolved
src/components/DataSourcePicker/DataSourcePicker.tsx Outdated Show resolved Hide resolved
src/components/DataLinks/DataLinks.test.tsx Outdated Show resolved Hide resolved
src/components/DataLinks/DataLinks.test.tsx Show resolved Hide resolved
src/components/DataLinks/DataLinks.test.tsx Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
babel.config.json Outdated Show resolved Hide resolved
src/components/DataLinks/DataLinks.test.tsx Outdated Show resolved Hide resolved
srclosson and others added 7 commits December 11, 2020 08:51
Co-authored-by: Vicky Lee <36230812+vickyyyyyyy@users.noreply.github.com>
Co-authored-by: Vicky Lee <36230812+vickyyyyyyy@users.noreply.github.com>
@srclosson srclosson merged commit 79e8022 into master Jan 5, 2021
@srclosson srclosson deleted the src/add-datasource-picker branch January 5, 2021 16:52
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

4 participants