Skip to content

Conversation

@kishaningithub
Copy link
Contributor

@kishaningithub kishaningithub commented Mar 18, 2022

@kishaningithub kishaningithub requested a review from a team as a code owner March 18, 2022 11:29
@kishaningithub kishaningithub requested review from fridgepoet and iwysiu and removed request for a team March 18, 2022 11:29
Copy link
Contributor

@andresmgot andresmgot left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @kishaningithub!

It should be possible to opt-out of this feature though. I think this setting is suitable to be included in the data source configuration page.

For doing so, it's necessary to:

import { InlineField, Switch } from '@grafana/ui';

...

      <InlineField label="Send events to Amazon EventBridge" labelWidth={28}>
        <Switch
          value={props.options.jsonData.withEvent}
          onChange={(e) =>
            props.onOptionsChange({
              ...props.options,
              jsonData: {
                ...props.options.jsonData,
                withEvent: e.currentTarget.checked,
              },
            })
          }
        />
      </InlineField>

@kishaningithub kishaningithub force-pushed the feature-add-monitoring-support-via-event-bridge branch from 5275c85 to 97453b0 Compare March 21, 2022 13:18
@kishaningithub
Copy link
Contributor Author

@andresmgot Have addressed your feedback. Thanks for the detailed feedback though :-)

Copy link
Contributor

@andresmgot andresmgot left a comment

Choose a reason for hiding this comment

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

Great work here @kishaningithub! Only a couple more of minor comments to go.

label={selectors.components.ConfigEditor.Database.input}
data-testid={selectors.components.ConfigEditor.Database.testID}
/>
<InlineField label="Send events to Amazon EventBridge" labelWidth={28}>
Copy link
Contributor

Choose a reason for hiding this comment

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

If you add style={{ alignItems: 'center' }} the switch will be vertically aligned

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What is the easiest a way to view how the UI will look like in local machine? I tried yarn dev and yarn watch but they seem to be focussed on unit tests

Copy link
Contributor

Choose a reason for hiding this comment

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

I use yarn watch, that shouldn't run the tests. With more details, what I run in development is:

  • make run in the main grafana/grafana repository
  • yarn start in grafana/grafana
  • grafana/grafana/data/plugins/redshift-datasource is a symbolic link pointing to my Redshift dist folder (grafana/redshift-datasource/dist)
  • yarn watch in grafana/redshift-datasource

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@andresmgot What do you think of adding the above steps to CONTRIBUTING.md? Would be useful for people who are new to grafana plugin development like me :-)

Copy link
Contributor

Choose a reason for hiding this comment

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

that's a good point. Let me check if that's already written somewhere and if not I will add it there

<Switch
value={props.options.jsonData.withEvent ?? false}
onChange={(e) =>
props.onOptionsChange({
Copy link
Contributor

Choose a reason for hiding this comment

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

can you add a simple test for this? If you go to ConfigEditor.test.tsx you can take inspiration from the test should allow user to enter a database. You will need to add a data-testid to this Switch to test it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@andresmgot I have added test for this, If you see the test currently fails because the onChange is not getting triggered. I am unable to find a reason for this. Need your help in getting this test case passing. Thanks in advance! :-)

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's because how Switch is internally implemented, if you do fireEvent.click(withEventField); rather than fireEvent.change(withEventField, { target: { checked: true } }); it should work.

@kishaningithub kishaningithub force-pushed the feature-add-monitoring-support-via-event-bridge branch from 4457d95 to c75745a Compare March 22, 2022 01:08
Copy link
Contributor

@andresmgot andresmgot left a comment

Choose a reason for hiding this comment

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

Thanks for the changes!

<Switch
value={props.options.jsonData.withEvent ?? false}
onChange={(e) =>
props.onOptionsChange({
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's because how Switch is internally implemented, if you do fireEvent.click(withEventField); rather than fireEvent.change(withEventField, { target: { checked: true } }); it should work.

@kishaningithub
Copy link
Contributor Author

@andresmgot Have addressed your review comments.. Thanks for the detailed reviews :-)

Copy link
Contributor

@andresmgot andresmgot left a comment

Choose a reason for hiding this comment

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

Great! Thanks for the changes.

@andresmgot andresmgot merged commit 868ed15 into grafana:main Mar 24, 2022
@fridgepoet fridgepoet mentioned this pull request Mar 24, 2022
@kishaningithub kishaningithub deleted the feature-add-monitoring-support-via-event-bridge branch March 24, 2022 10:00
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.

2 participants