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

refactor(react-storage): Primary to composable ActionStart #5935

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
import React from 'react';
import { ButtonElement } from '../context/elements';
Copy link
Contributor

@reesscot reesscot Oct 30, 2024

Choose a reason for hiding this comment

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

nit: we typically add an empty line between third party imports and local imports

Suggested change
import { ButtonElement } from '../context/elements';
import { ButtonElement } from '../context/elements';

(really need to just have an eslint rule for this)

import { CLASS_BASE } from '../views/constants';

export interface ActionStartProps {
onStart?: () => void;
isDisabled?: boolean;
label?: string;
}

export const ActionStart = ({
onStart,
isDisabled,
label,
}: ActionStartProps): React.JSX.Element => (
<ButtonElement
variant="primary"
className={`${CLASS_BASE}__action-start`}
onClick={onStart}
disabled={isDisabled}
>
{label}
</ButtonElement>
);
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
import React from 'react';
import { render, screen } from '@testing-library/react';
import { ActionStart } from '../ActionStart';
import { CLASS_BASE } from '../../views/constants';

describe('ActionStart', () => {
it('renders a button element', () => {
render(<ActionStart />);
const button = screen.getByRole('button');
expect(button).toBeInTheDocument();
});

it('renders a button with the expected className', () => {
render(<ActionStart />);
const button = screen.getByRole('button');
expect(button).toHaveClass(`${CLASS_BASE}__action-start`);
});

it('renders a button with the expected text', () => {
render(<ActionStart label="Start" />);
const button = screen.getByRole('button');
expect(button).toHaveTextContent('Start');
});

it('renders a button with the expected disabled state', () => {
render(<ActionStart isDisabled />);
const button = screen.getByRole('button');
expect(button).toBeDisabled();
});
});
Original file line number Diff line number Diff line change
@@ -1,11 +1,13 @@
import { DataTableProps } from './DataTable';
import { StatusDisplayProps } from './StatusDisplay';
import { DataRefreshProps } from './DataRefresh';
import { ActionStartProps } from './ActionStart';

export interface Composables {
DataRefresh: React.ComponentType<DataRefreshProps>;
DataTable: React.ComponentType<DataTableProps>;
StatusDisplay: React.ComponentType<StatusDisplayProps>;
ActionStart: React.ComponentType<ActionStartProps>;
}

export interface ComposablesContext {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
import React from 'react';

import { ControlProps } from './types';
import { useResolvedComposable } from './hooks/useResolvedComposable';
import { ActionStart } from '../composables/ActionStart';
import { useActionStart } from './hooks/useActionStart';
import { ViewElement } from '../context/elements';

export const ActionStartControl = ({
className,
}: ControlProps): React.JSX.Element | null => {
const props = useActionStart();
const ResolvedActionStart = useResolvedComposable(ActionStart, 'ActionStart');

return (
<ViewElement className={className}>
<ResolvedActionStart {...props} />
</ViewElement>
);
};
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
import React from 'react';
import { render, screen } from '@testing-library/react';
import { ActionStartControl } from '../ActionStartControl';
import * as useActionStartModule from '../hooks/useActionStart';

describe('ActionStartControl', () => {
const useActionStartSpy = jest.spyOn(useActionStartModule, 'useActionStart');

afterEach(() => {
useActionStartSpy.mockClear();
});

afterAll(() => {
useActionStartSpy.mockRestore();
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this necessary, aren't the tests already isolated?

});

it('renders', () => {
useActionStartSpy.mockReturnValue({
isDisabled: false,
onStart: jest.fn(),
label: 'Start',
});
render(<ActionStartControl />);

const button = screen.getByRole('button', {
name: 'Start',
});

expect(button).toBeInTheDocument();
});

it('renders with custom label', () => {
useActionStartSpy.mockReturnValue({
isDisabled: false,
onStart: jest.fn(),
label: 'Custom Label',
});
render(<ActionStartControl />);

const button = screen.getByRole('button', {
name: 'Custom Label',
});

expect(button).toBeInTheDocument();
});

it('calls onStart when button is clicked', () => {
const mockOnStart = jest.fn();
useActionStartSpy.mockReturnValue({
isDisabled: false,
onStart: mockOnStart,
label: 'Start',
});
render(<ActionStartControl />);

const button = screen.getByRole('button', {
name: 'Start',
});

button.click();

expect(mockOnStart).toHaveBeenCalled();
});
});
Samaritan1011001 marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
import * as controlsContextModule from '../../../controls/context';
import { ControlsContext } from '../../types';
import { useActionStart } from '../useActionStart';

describe('useActionStart', () => {
const controlsContext: ControlsContext = {
data: {
actionStartLabel: 'Start',
},
actionsConfig: {
isCancelable: true,
type: 'BATCH_ACTION',
},
Comment on lines +10 to +13
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
actionsConfig: {
isCancelable: true,
type: 'BATCH_ACTION',
},

Copy link
Member

Choose a reason for hiding this comment

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

nit:
same as comment above

onActionStart: jest.fn(),
};

const useControlsContextSpy = jest.spyOn(
controlsContextModule,
'useControlsContext'
);

afterEach(() => {
useControlsContextSpy.mockClear();
});

afterAll(() => {
useControlsContextSpy.mockRestore();
});

it('returns object as it is received from ControlsContext', () => {
useControlsContextSpy.mockReturnValue(controlsContext);

expect(useActionStart()).toStrictEqual({
label: controlsContext.data.actionStartLabel,
onStart: expect.any(Function),
isDisabled: controlsContext.data.isActionStartDisabled,
});
});

it('calls onActionStart from ControlsContext when onStart is called', () => {
useControlsContextSpy.mockReturnValue(controlsContext);

const { onStart } = useActionStart();
onStart!();

expect(controlsContext.onActionStart).toHaveBeenCalledTimes(1);
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
import { ActionStartProps } from '../../composables/ActionStart';
import { useControlsContext } from '../../controls/context';

export const useActionStart = (): ActionStartProps => {
const {
data: { actionStartLabel, isActionStartDisabled },
onActionStart,
} = useControlsContext();
return {
label: actionStartLabel,
isDisabled: isActionStartDisabled,
onStart: onActionStart,
};
};
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@ export interface ControlsContext {
taskCounts?: TaskCounts;
tableData?: TableData;
isDataRefreshDisabled?: boolean;
actionStartLabel?: string;
isActionStartDisabled?: boolean;
};
actionsConfig?: {
type:
Expand All @@ -45,4 +47,5 @@ export interface ControlsContext {
isCancelable?: boolean;
};
onRefresh?: () => void;
onActionStart?: () => void;
}
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ import { MessageControl } from './Message';
import { NavigateControl } from './Navigate';
import { OverwriteControl } from './Overwrite';
import { PaginateControl } from './Paginate';
import { PrimaryControl } from './Primary';
import { SearchControl } from './Search';
import { TableControl } from './Table';
import { TitleControl } from './Title';
Expand All @@ -21,7 +20,6 @@ export interface Controls {
Message: typeof MessageControl;
Overwrite: typeof OverwriteControl;
Paginate: typeof PaginateControl;
Primary: typeof PrimaryControl;
Navigate: typeof NavigateControl;
Search: typeof SearchControl;
Table: typeof TableControl;
Expand All @@ -37,7 +35,6 @@ export const Controls: Controls = {
Message: MessageControl,
Overwrite: OverwriteControl,
Paginate: PaginateControl,
Primary: PrimaryControl,
Navigate: NavigateControl,
Search: SearchControl,
Table: TableControl,
Expand Down

This file was deleted.

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ export { MessageControl } from './Message';
export { NavigateControl, NavigateItem } from './Navigate';
export { OverwriteControl } from './Overwrite';
export { PaginateControl } from './Paginate';
export { PrimaryControl } from './Primary';
export { SearchControl } from './Search';
export { TitleControl } from './Title';
export { TableControl, LocationDetailViewTable } from './Table';
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,12 @@ import { useStore } from '../../providers/store';
import { Controls } from '../Controls';

import { Title } from './Controls/Title';
import { ActionStartControl } from '../../controls/ActionStartControl';
import { ControlsContext } from '../../controls/types';
import { ControlsContextProvider } from '../../controls/context';
import { CLASS_BASE } from '../constants';

const { Exit, Message, Primary } = Controls;
const { Exit, Message } = Controls;

export const isValidFolderName = (name: string | undefined): boolean =>
!!name?.length && !name.includes('/');
Expand Down Expand Up @@ -88,31 +92,34 @@ export const CreateFolderControls = ({
handleCreateAction({ prefix: '', options: { reset: true } });
};

const primaryProps =
result?.status === 'COMPLETE'
? {
onClick: () => {
handleClose();
},
children: 'Folder created',
}
: {
onClick: () => {
handleCreateFolder();
},
children: 'Create Folder',
disabled: !folderName || !!fieldValidationError,
};
const hasCompletedStatus = result?.status === 'COMPLETE';

// FIXME: Eventually comes from useView hook
const contextValue: ControlsContext = {
data: {
actionStartLabel: hasCompletedStatus ? 'Folder created' : 'Create Folder',
isActionStartDisabled: !hasCompletedStatus
? !folderName || !!fieldValidationError
: undefined,
},
actionsConfig: {
type: 'SINGLE_ACTION',
isCancelable: true,
},
Comment on lines +105 to +108
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
actionsConfig: {
type: 'SINGLE_ACTION',
isCancelable: true,
},

Copy link
Member

Choose a reason for hiding this comment

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

nit
actionsConfig is optional, can be removed if not required

onActionStart: hasCompletedStatus ? handleClose : handleCreateFolder,
};

return (
<>
<ControlsContextProvider {...contextValue}>
<Exit
onClick={() => {
handleClose();
}}
/>
<Title />
<Primary {...primaryProps} />
<ActionStartControl
className={`${CLASS_BASE}__create-folder-action-start`}
/>
<Field
label="Enter folder name:"
disabled={isLoading || !!result?.status}
Expand All @@ -132,6 +139,6 @@ export const CreateFolderControls = ({
{result?.status === 'COMPLETE' || result?.status === 'FAILED' ? (
<CreateFolderMessage />
) : null}
</>
</ControlsContextProvider>
);
};
Loading
Loading