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

chore: hide table rows while results are loading #6089

Closed
wants to merge 32 commits into from
Closed

chore: hide table rows while results are loading #6089

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
6711204
chore: move loading indicator into table, hide table contents while l…
jordanvn Nov 14, 2024
662eaea
Merge branch 'feat-storage-browser/main' into feat-storage-browser/ta…
jordanvn Nov 14, 2024
9c6034d
place loading indicator outside of table, resize table based on contents
jordanvn Nov 15, 2024
e352bd6
Merge branch 'feat-storage-browser/main' into feat-storage-browser/ta…
jordanvn Nov 15, 2024
1491905
chore: update styling to allow full vertical scrolling for table
jordanvn Nov 15, 2024
0fac109
chore(tests): update tests to fit new approach to loading
jordanvn Nov 15, 2024
4538b9c
Merge branch 'feat-storage-browser/main' into feat-storage-browser/ta…
jordanvn Nov 15, 2024
04bca01
chore: move loading label to prop, return null instead of false, retu…
jordanvn Nov 15, 2024
4b9e70e
chore: removing duplicate data-testid
jordanvn Nov 15, 2024
56f4097
chore: moving data-table view out of table component and into data ta…
jordanvn Nov 15, 2024
fdd7a20
chore: move loading indicator element to data, process label at view …
jordanvn Nov 15, 2024
29b3617
fix(tests): update data in tests to include new changes
jordanvn Nov 15, 2024
bf3030e
chore: change loading indicator to react node, make isLoading optional
jordanvn Nov 15, 2024
b7e4da4
chore: pass in isLoading at view level, remove isLoading from tableData
jordanvn Nov 15, 2024
9cb48ef
chore: remove unnecessary usage of isLoading, destructure data
jordanvn Nov 15, 2024
82d996a
chore: use object shorthand
jordanvn Nov 15, 2024
24bd121
Merge branch 'feat-storage-browser/main' into feat-storage-browser/ta…
jordanvn Nov 15, 2024
ac34eda
Merge branch 'feat-storage-browser/main' into feat-storage-browser/ta…
jordanvn Nov 15, 2024
60887c5
Merge branch 'feat-storage-browser/main' into feat-storage-browser/ta…
jordanvn Nov 15, 2024
a30db2f
chore: removing loading indicator component in favor of composable
jordanvn Nov 15, 2024
d4adb68
Merge branch 'feat-storage-browser/main' into feat-storage-browser/ta…
jordanvn Nov 15, 2024
015a70a
Merge branch 'feat-storage-browser/main' into feat-storage-browser/ta…
jordanvn Nov 15, 2024
3077998
chore: set isLoading to true on loading indicator composables
jordanvn Nov 16, 2024
d3cd630
Merge branch 'feat-storage-browser/main' into feat-storage-browser/ta…
jordanvn Nov 16, 2024
93a796c
Merge branch 'feat-storage-browser/main' into feat-storage-browser/ta…
jordanvn Nov 16, 2024
e79f718
Merge branch 'feat-storage-browser/main' into feat-storage-browser/ta…
jordanvn Nov 16, 2024
013b42f
Merge branch 'feat-storage-browser/main' into feat-storage-browser/ta…
jordanvn Nov 18, 2024
b878f2d
Merge branch 'feat-storage-browser/main' into feat-storage-browser/ta…
jordanvn Nov 19, 2024
e376108
chore: removing usage of loading indicator inside table
jordanvn Nov 19, 2024
dcd69aa
chore: removing unnecessary usage of loading indicator and loading in…
jordanvn Nov 19, 2024
2ebf0e6
chore: reverting now-unnecessary style changes
jordanvn Nov 19, 2024
95fcac5
chore: remove unnecessary inclusion of optional isLoading prop, fix u…
jordanvn Nov 19, 2024
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
25 changes: 25 additions & 0 deletions packages/react-storage/src/components/StorageBrowser/components/LoadingIndicator.tsx
View file
Open in desktop
jordanvn marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
import React from 'react';

import { IconElement, SpanElement, ViewElement } from '../context/elements';
import { STORAGE_BROWSER_BLOCK } from '../constants';

export interface LoadingIndicatorProps {
label?: string;
}

export const LoadingIndicator = ({
label,
}: LoadingIndicatorProps): React.JSX.Element => (
<ViewElement className={`${STORAGE_BROWSER_BLOCK}__loading-indicator`}>
<IconElement
className={`${STORAGE_BROWSER_BLOCK}__loading-indicator-icon`}
variant="loading"
/>
<SpanElement
className={`${STORAGE_BROWSER_BLOCK}__loading-indicator-text`}
aria-live="polite"
>
{label}
</SpanElement>
</ViewElement>
);
77 changes: 37 additions & 40 deletions packages/react-storage/src/components/StorageBrowser/components/Table.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ import {
TableHeadElement,
TableHeaderElement,
TableRowElement,
ViewElement,
} from '../context/elements';

import { STORAGE_BROWSER_BLOCK } from '../constants';
Expand All @@ -29,50 +28,48 @@ interface TableProps {

export const Table = ({ headers, rows }: TableProps): React.JSX.Element => {
return (
<ViewElement className={`${STORAGE_BROWSER_BLOCK}__data-table`}>
<TableElement className={`${STORAGE_BROWSER_BLOCK}__table`}>
<TableHeadElement className={`${STORAGE_BROWSER_BLOCK}__table-head`}>
{headers.length ? (
<TableRowElement className={`${STORAGE_BROWSER_BLOCK}__table-row`}>
{headers.map(({ key, content }) => (
jordanvn marked this conversation as resolved.
Show resolved Hide resolved
<TableElement className={`${STORAGE_BROWSER_BLOCK}__table`}>
<TableHeadElement className={`${STORAGE_BROWSER_BLOCK}__table-head`}>
{headers.length ? (
<TableRowElement className={`${STORAGE_BROWSER_BLOCK}__table-row`}>
{headers.map(({ key, content }) => (
<TableHeaderElement
key={key}
className={`${STORAGE_BROWSER_BLOCK}__table-header`}
>
{content}
</TableHeaderElement>
))}
</TableRowElement>
) : null}
</TableHeadElement>
<TableBodyElement className={`${STORAGE_BROWSER_BLOCK}__table-body`}>
{rows?.map(({ key, content }) => (
<TableRowElement
key={key}
className={`${STORAGE_BROWSER_BLOCK}__table-row`}
>
{content.map(({ key, content, type }) => {
return type === 'header' ? (
<TableHeaderElement
key={key}
className={`${STORAGE_BROWSER_BLOCK}__table-header`}
role="rowheader"
>
{content}
</TableHeaderElement>
))}
</TableRowElement>
) : null}
</TableHeadElement>
<TableBodyElement className={`${STORAGE_BROWSER_BLOCK}__table-body`}>
{rows?.map(({ key, content }) => (
<TableRowElement
key={key}
className={`${STORAGE_BROWSER_BLOCK}__table-row`}
>
{content.map(({ key, content, type }) => {
return type === 'header' ? (
<TableHeaderElement
key={key}
className={`${STORAGE_BROWSER_BLOCK}__table-header`}
role="rowheader"
>
{content}
</TableHeaderElement>
) : (
<TableDataCellElement
key={key}
className={`${STORAGE_BROWSER_BLOCK}__table-data-cell`}
>
{content}
</TableDataCellElement>
);
})}
</TableRowElement>
))}
jordanvn marked this conversation as resolved.
Show resolved Hide resolved
</TableBodyElement>
</TableElement>
</ViewElement>
) : (
<TableDataCellElement
key={key}
className={`${STORAGE_BROWSER_BLOCK}__table-data-cell`}
>
{content}
</TableDataCellElement>
);
})}
</TableRowElement>
))}
</TableBodyElement>
</TableElement>
);
};
13 changes: 13 additions & 0 deletions ...eact-storage/src/components/StorageBrowser/components/__tests__/LoadingIndicator.spec.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
import React from 'react';
import { render, screen } from '@testing-library/react';
import { LoadingIndicator } from '../LoadingIndicator';

describe('LoadingIndicator', () => {
it('renders', () => {
render(<LoadingIndicator label="Loading" />);

const indicator = screen.getByText('Loading');

expect(indicator).toBeInTheDocument();
});
});
91 changes: 52 additions & 39 deletions packages/react-storage/src/components/StorageBrowser/composables/DataTable/DataTable.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,18 +11,24 @@ import { TextDataCell } from './dataCells/TextDataCell';
import { DataTableDataCell, DataTableHeader } from './types';
import { WithKey } from '../../components/types';
import { CheckboxHeader } from './headers/CheckboxHeader';
import { STORAGE_BROWSER_BLOCK } from '../../constants';
import { ViewElement } from '../../context/elements';

export interface DataTableRow {
content: WithKey<DataTableDataCell>[];
}

export interface DataTableProps {
headers: WithKey<DataTableHeader>[];
isLoading: boolean;
Copy link
Member

Choose a reason for hiding this comment

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

I think it's ok to keep this optional and just treat undefined as falsy?

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated

loadingIndicator?: React.JSX.Element;
Copy link
Member

Choose a reason for hiding this comment

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

Nit: I don't have strong arguments for or against this but should this be a ReactNode?

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated to React.ReactNode

rows: WithKey<DataTableRow>[];
}

export const DataTable = ({
headers,
isLoading,
loadingIndicator,
rows,
}: DataTableProps): React.JSX.Element => {
const mappedHeaders = headers.map(({ key, content, type }) => {
Expand All @@ -49,44 +55,51 @@ export const DataTable = ({
}
});

const mappedRows = rows.map(({ key, content }) => ({
key,
content: content.map(({ key, content, type }) => {
switch (type) {
case 'button': {
return {
key,
content: <ButtonDataCell content={content} />,
};
}
case 'checkbox': {
return {
key,
content: <CheckboxDataCell content={content} />,
};
}
case 'date': {
return {
key,
content: <DateDataCell content={content} />,
};
}
case 'number': {
return {
key,
content: <NumberDataCell content={content} />,
};
}
case 'text':
default: {
return {
key,
content: <TextDataCell content={content} />,
};
}
}
}),
}));
Comment on lines -52 to -89
Copy link
Member Author

Choose a reason for hiding this comment

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

There is no change to this section, it is just wrapped in conditional logic

const mappedRows = isLoading
? []
: rows.map(({ key, content }) => ({
key,
content: content.map(({ key, content, type }) => {
switch (type) {
case 'button': {
return {
key,
content: <ButtonDataCell content={content} />,
};
}
case 'checkbox': {
return {
key,
content: <CheckboxDataCell content={content} />,
};
}
case 'date': {
return {
key,
content: <DateDataCell content={content} />,
};
}
case 'number': {
return {
key,
content: <NumberDataCell content={content} />,
};
}
case 'text':
default: {
return {
key,
content: <TextDataCell content={content} />,
};
}
}
}),
}));

return <Table headers={mappedHeaders} rows={mappedRows} />;
return (
<ViewElement className={`${STORAGE_BROWSER_BLOCK}__data-table`}>
<Table headers={mappedHeaders} rows={mappedRows} />
{isLoading ? loadingIndicator : null}
</ViewElement>
);
};
2 changes: 1 addition & 1 deletion ...-storage/src/components/StorageBrowser/composables/__tests__/DataTable/DataTable.spec.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ describe('DataTable', () => {
},
];

render(<DataTable headers={headers} rows={rows} />);
render(<DataTable headers={headers} isLoading={false} rows={rows} />);

const table = screen.getByRole('table');
const tableRowGroups = screen.getAllByRole('rowgroup');
Expand Down
25 changes: 24 additions & 1 deletion ...react-storage/src/components/StorageBrowser/controls/hooks/__tests__/useDataTable.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,8 @@ describe('useDataTable', () => {
{ key: 'header-4', ...textHeader },
{ key: 'header-5', ...textHeader },
],
isLoading: false,
loadingIndicator: undefined,
rows: [
{
key: 'row-1',
Expand Down Expand Up @@ -88,6 +90,8 @@ describe('useDataTable', () => {
{ key: 'header-4', ...textHeader },
{ key: 'header-5', ...textHeader },
],
isLoading: false,
loadingIndicator: undefined,
rows: [
{
key: 'row-1',
Expand All @@ -108,14 +112,21 @@ describe('useDataTable', () => {

const { result } = renderHook(() => useDataTable());

expect(result.current).toStrictEqual({ headers: [], rows: [] });
expect(result.current).toStrictEqual({
headers: [],
isLoading: false,
loadingIndicator: undefined,
rows: [],
});
});

it('handles data with no sortable columns', () => {
mockUseControlsContext.mockReturnValue({
data: {
tableData: {
headers: [{ key: 'header-1', ...textHeader }],
isLoading: false,
loadingIndicator: undefined,
rows: [
{
key: 'row-1',
Expand All @@ -130,6 +141,8 @@ describe('useDataTable', () => {

expect(result.current).toStrictEqual({
headers: [expect.objectContaining({ key: 'header-1' })],
isLoading: false,
loadingIndicator: undefined,
rows: [expect.objectContaining({ key: 'row-1' })],
});
});
Expand All @@ -143,6 +156,8 @@ describe('useDataTable', () => {
{ key: 'header-2', ...sortHeader },
{ key: 'header-3', ...sortHeader },
],
isLoading: false,
loadingIndicator: undefined,
rows: [
{
key: 'row-1',
Expand Down Expand Up @@ -189,6 +204,8 @@ describe('useDataTable', () => {
{ key: 'header-2', ...sortHeader },
{ key: 'header-3', ...sortHeader },
],
isLoading: false,
loadingIndicator: undefined,
rows: [
{
key: 'row-1',
Expand Down Expand Up @@ -262,6 +279,8 @@ describe('useDataTable', () => {
data: {
tableData: {
headers: [{ key: 'header-1', ...sortHeader }],
isLoading: false,
loadingIndicator: undefined,
rows: [
{
key: 'row-1',
Expand Down Expand Up @@ -297,6 +316,8 @@ describe('useDataTable', () => {
data: {
tableData: {
headers: [{ key: 'header-1', ...sortHeader }],
isLoading: false,
loadingIndicator: undefined,
rows: [
{
key: 'row-1',
Expand Down Expand Up @@ -332,6 +353,8 @@ describe('useDataTable', () => {
data: {
tableData: {
headers: [{ key: 'header-1', ...sortHeader }],
isLoading: false,
loadingIndicator: undefined,
rows: [
{
key: 'row-1',
Expand Down
2 changes: 2 additions & 0 deletions packages/react-storage/src/components/StorageBrowser/controls/hooks/useDataTable.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,6 +154,8 @@ export const useDataTable = (): DataTableProps => {

return {
headers: mappedHeaders ?? [],
isLoading: !!data.isDataRefreshDisabled,
Copy link
Member

@cshfang cshfang Nov 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Assuming this is made optional, wouldn't need the !!

But, more importantly, I think this can be driven off of isLoading?

  const { data } = useControlsContext();
  const { isLoading, loadingIndicator, tableData } = data;

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, removed after updating isLoading to be optional

loadingIndicator: data.loadingIndicator,
rows: sortedRows ?? [],
};
};
1 change: 1 addition & 0 deletions packages/react-storage/src/components/StorageBrowser/controls/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ export interface ControlsContext {
isFolderNameDisabled?: boolean;
isOverwriteToggleDisabled?: boolean;
isSearchingSubfolders?: boolean;
loadingIndicator?: React.JSX.Element;
Copy link
Member

Choose a reason for hiding this comment

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

This needs to a React.ReactNode

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated to React.ReactNode

loadingIndicatorLabel?: string;
location?: LocationState;
overwriteToggleLabel?: string;
Expand Down
Loading
Loading