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.

Sign up for GitHub

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

[Drilldowns] Preserve state when switching between action factories #65074

Merged
merged 3 commits into from
May 7, 2020
Merged

[Drilldowns] Preserve state when switching between action factories #65074

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
09599fd
preserve state when selecting different action factory
Dosant May 4, 2020
6a76add
Merge branch 'master' into dev/preserve-state-in-action-wizard
elasticmachine May 6, 2020
642cfac
Merge branch 'master' into dev/preserve-state-in-action-wizard
elasticmachine May 7, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions x-pack/plugins/advanced_ui_actions/public/components/action_wizard/action_wizard.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,9 +32,9 @@ export interface ActionWizardProps {

/**
* Action factory selected changed
* null - means user click "change" and removed action factory selection
* empty - means user click "change" and removed action factory selection
*/
onActionFactoryChange: (actionFactory: ActionFactory | null) => void;
onActionFactoryChange: (actionFactory?: ActionFactory) => void;

/**
* current config for currently selected action factory
Expand Down Expand Up @@ -71,7 +71,7 @@ export const ActionWizard: React.FC<ActionWizardProps> = ({
actionFactory={currentActionFactory}
showDeselect={actionFactories.length > 1}
onDeselect={() => {
onActionFactoryChange(null);
onActionFactoryChange(undefined);
}}
context={context}
config={config}
Expand Down
2 changes: 1 addition & 1 deletion x-pack/plugins/advanced_ui_actions/public/components/action_wizard/test_data.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,7 @@ export function Demo({ actionFactories }: { actionFactories: Array<ActionFactory
config?: ActionBaseConfig;
}>({});

function changeActionFactory(newActionFactory: ActionFactory | null) {
function changeActionFactory(newActionFactory?: ActionFactory) {
if (!newActionFactory) {
// removing action factory
return setState({});
Expand Down
36 changes: 36 additions & 0 deletions ...components/connected_flyout_manage_drilldowns/connected_flyout_manage_drilldowns.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,6 +173,42 @@ test('Create only mode', async () => {
expect(await mockDynamicActionManager.state.get().events.length).toBe(1);
});

test('After switching between action factories state is restored', async () => {
const screen = render(
<FlyoutManageDrilldowns
placeContext={{}}
dynamicActionManager={mockDynamicActionManager}
viewMode={'create'}
/>
);
// wait for initial render. It is async because resolving compatible action factories is async
await wait(() => expect(screen.getAllByText(/Create/i).length).toBeGreaterThan(0));
fireEvent.change(screen.getByLabelText(/name/i), {
target: { value: 'test' },
});
fireEvent.click(screen.getByText(/Go to URL/i));
fireEvent.change(screen.getByLabelText(/url/i), {
target: { value: 'https://elastic.co' },
});

// change to dashboard
fireEvent.click(screen.getByText(/change/i));
fireEvent.click(screen.getByText(/Go to Dashboard/i));

// change back to url
fireEvent.click(screen.getByText(/change/i));
fireEvent.click(screen.getByText(/Go to URL/i));

expect(screen.getByLabelText(/url/i)).toHaveValue('https://elastic.co');
expect(screen.getByLabelText(/name/i)).toHaveValue('test');

fireEvent.click(screen.getAllByText(/Create Drilldown/i)[1]);
await wait(() => expect(notifications.toasts.addSuccess).toBeCalled());
expect(await (mockDynamicActionManager.state.get().events[0].action.config as any).url).toBe(
'https://elastic.co'
);
});

test.todo("Error when can't fetch drilldown list");

test("Error when can't save drilldown changes", async () => {
Expand Down
103 changes: 71 additions & 32 deletions .../plugins/drilldowns/public/components/flyout_drilldown_wizard/flyout_drilldown_wizard.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,72 @@ export interface FlyoutDrilldownWizardProps<CurrentActionConfig extends object =
actionFactoryContext?: object;
}

function useWizardConfigState(
initialDrilldownWizardConfig?: DrilldownWizardConfig
): [
DrilldownWizardConfig,
{
setName: (name: string) => void;
setActionConfig: (actionConfig: object) => void;
setActionFactory: (actionFactory?: ActionFactory) => void;
}
] {
const [wizardConfig, setWizardConfig] = useState<DrilldownWizardConfig>(
() =>
initialDrilldownWizardConfig ?? {
name: '',
}
);
const [actionConfigCache, setActionConfigCache] = useState<Record<string, object>>(
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about having all configs in DrilldownWizardConfig instead of introducing actionConfigCache?

export interface DrilldownWizardConfig {
  name: string;
  actionFactory?: ActionFactory;
  configs?: Record<string, object>;
}

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 think I like the current version more, because we leave api of this component simpler, but still have this nice UX touch of not losing changes when switching between factories.

I wouldn't do it unless we really want to preserve those internal states longer then component lifecycle.

initialDrilldownWizardConfig?.actionFactory
? {
[initialDrilldownWizardConfig.actionFactory
.id]: initialDrilldownWizardConfig.actionConfig!,
}
: {}
);

return [
wizardConfig,
{
setName: (name: string) => {
setWizardConfig({
...wizardConfig,
name,
});
},
setActionConfig: (actionConfig: object) => {
setWizardConfig({
...wizardConfig,
actionConfig,
});
},
setActionFactory: (actionFactory?: ActionFactory) => {
if (actionFactory) {
setWizardConfig({
...wizardConfig,
actionFactory,
actionConfig: actionConfigCache[actionFactory.id] ?? actionFactory.createConfig(),
});
} else {
if (wizardConfig.actionFactory?.id) {
setActionConfigCache({
...actionConfigCache,
[wizardConfig.actionFactory.id]: wizardConfig.actionConfig!,
});
}

setWizardConfig({
...wizardConfig,
actionFactory: undefined,
actionConfig: undefined,
});
}
},
},
];
}

export function FlyoutDrilldownWizard<CurrentActionConfig extends object = object>({
onClose,
onBack,
Expand All @@ -53,11 +119,8 @@ export function FlyoutDrilldownWizard<CurrentActionConfig extends object = objec
drilldownActionFactories,
actionFactoryContext,
}: FlyoutDrilldownWizardProps<CurrentActionConfig>) {
const [wizardConfig, setWizardConfig] = useState<DrilldownWizardConfig>(
() =>
initialDrilldownWizardConfig ?? {
name: '',
}
const [wizardConfig, { setActionFactory, setActionConfig, setName }] = useWizardConfigState(
initialDrilldownWizardConfig
);

const isActionValid = (
Expand Down Expand Up @@ -95,35 +158,11 @@ export function FlyoutDrilldownWizard<CurrentActionConfig extends object = objec
>
<FormDrilldownWizard
name={wizardConfig.name}
onNameChange={newName => {
setWizardConfig({
...wizardConfig,
name: newName,
});
}}
onNameChange={setName}
actionConfig={wizardConfig.actionConfig}
onActionConfigChange={newActionConfig => {
setWizardConfig({
...wizardConfig,
actionConfig: newActionConfig,
});
}}
onActionConfigChange={setActionConfig}
currentActionFactory={wizardConfig.actionFactory}
onActionFactoryChange={actionFactory => {
if (!actionFactory) {
setWizardConfig({
...wizardConfig,
actionFactory: undefined,
actionConfig: undefined,
});
} else {
setWizardConfig({
...wizardConfig,
actionFactory,
actionConfig: actionFactory.createConfig(),
});
}
}}
onActionFactoryChange={setActionFactory}
actionFactories={drilldownActionFactories}
actionFactoryContext={actionFactoryContext!}
/>
Expand Down
2 changes: 1 addition & 1 deletion x-pack/plugins/drilldowns/public/components/form_drilldown_wizard/form_drilldown_wizard.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ export interface FormDrilldownWizardProps {
onNameChange?: (name: string) => void;

currentActionFactory?: ActionFactory;
onActionFactoryChange?: (actionFactory: ActionFactory | null) => void;
onActionFactoryChange?: (actionFactory?: ActionFactory) => void;
actionFactoryContext: object;

actionConfig?: object;
Expand Down