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

Toast on CustomPanels #415

Closed
wants to merge 3 commits into from
Closed

Toast on CustomPanels #415

wants to merge 3 commits into from

Conversation

pjfitzgibbons
Copy link
Contributor

Description

Apply OSD GlobalToast to CustomPanels

Issues Resolved

Check List

  • New functionality includes testing.
    • All tests pass, including unit test, integration test and doctest
  • New functionality has been documented.
    • New functionality has javadoc added
    • New functionality has user manual doc added
  • Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Peter Fitzgibbons added 3 commits April 21, 2023 16:01
* Panel View (legacy)
  - Duplicate
  - Rename

Signed-off-by: Peter Fitzgibbons <[email protected]>
Signed-off-by: Peter Fitzgibbons <[email protected]>
@@ -77,7 +77,7 @@ describe('Creating visualizations', () => {
});
});

describe('Testing panels table', () => {
describe.only('Testing panels table', () => {
Copy link
Member

Choose a reason for hiding this comment

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

I feel the .only, commented code and console.log additions were part of cypress debugging. We can remove these.

@@ -222,17 +230,16 @@ export const CustomPanelTable = ({
'Duplicate Dashboard',
'Cancel',
'Duplicate',
selectedCustomPanels[0].title + ' (copy)',
selectedCustomPanels[0].title + ' (copy)x',
Copy link
Member

Choose a reason for hiding this comment

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

We can remove the x typo here.

services: { toasts },
} = useOpenSearchDashboards<ObservabilityAppServices>();

const setToast = (title: string, color: Color = 'success', text?, side?: string) => {
Copy link
Member

Choose a reason for hiding this comment

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

We can remove the side parameter, if not being used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Side is used when toast during flyout. So unless toast on right-side is OK while visualization flyout and others are active, only then could we remove it.

Side is being passed-through on OSD API

Copy link
Member

Choose a reason for hiding this comment

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

I may be missing something here. Can you please explain how are you using/passing down the side parameter in this function?

addSamplePanels,
}: Props) => {
const customPanels = useSelector<CustomPanelType[]>(selectPanelList);
const [isModalVisible, setIsModalVisible] = useState(false); // Modal Toggle
const [modalLayout, setModalLayout] = useState(<EuiOverlayMask />); // Modal Layout
const [isActionsPopoverOpen, setIsActionsPopoverOpen] = useState(false);
const [selectedCustomPanels, setselectedCustomPanels] = useState<CustomPanelType[]>([]);
const [selectedCustomPanels, setselectedCustomPanels$] = useState<CustomPanelType[]>([]);
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 if the $ is expected here. I see the below reference is setselectedCustomPanels

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There was debugging. Will double-check.

Comment on lines +126 to +128
const newPanel = newPanelTemplate(newCustomPanelName);
dispatch(createPanel(newPanel));
}
Copy link
Member

Choose a reason for hiding this comment

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

Should we add a toast for create panel here?
From the change in panel_slice.tsx, I see that we are not redirecting to a new panel when created. So, the user stays in home table page after the panel is created.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should be redirecting - that is the current behavior?

'Error cloning Operational Panel, please make sure you have the correct permission.',
'danger'
);
console.log(err);
Copy link
Member

Choose a reason for hiding this comment

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

We should replace console.log with console.error to show errors.

@pjfitzgibbons pjfitzgibbons changed the base branch from 2.7 to main April 24, 2023 16:42
@pjfitzgibbons pjfitzgibbons changed the base branch from main to 2.7 April 24, 2023 16:50
@@ -111,27 +134,31 @@ describe('Panels View Component', () => {
window.location.assign(`#/event_analytics/explorer/${savedVisId}`);
};

const services = { toasts: { addDanger: (t) => {} } }
Copy link
Member

Choose a reason for hiding this comment

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

@@ -0,0 +1,40 @@
import { createSlice } from '@reduxjs/toolkit';
Copy link
Member

Choose a reason for hiding this comment

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

license header missing

@@ -137,6 +138,10 @@ export class ObservabilityPlugin
const savedObjects = new SavedObjects(coreStart.http);
const timestampUtils = new TimestampUtils(dslService, pplService);

const services: ObservabilityAppServices = {
toasts: coreStart.notifications.toasts,
};
Copy link
Member

Choose a reason for hiding this comment

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

why is this another service and not using your coreRefs?

@joshuali925
Copy link
Member

also backport to main?

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.

4 participants