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

Jump to bottom

Optimize References Loaders Display #5668

Merged
merged 4 commits into from
Dec 18, 2020
Merged

Optimize References Loaders Display #5668

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a2dc263
Optimize References Loaders Display
djhi Dec 14, 2020
d14faf2
Fix tests
djhi Dec 15, 2020
108aaaf
Apply review
djhi Dec 15, 2020
df48a4d
Apply review
djhi Dec 18, 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
4 changes: 3 additions & 1 deletion packages/ra-ui-materialui/src/field/ReferenceArrayField.spec.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import SingleFieldList from '../list/SingleFieldList';

describe('<ReferenceArrayField />', () => {
afterEach(cleanup);
it('should render a loading indicator when related records are not yet fetched', () => {
it('should render a loading indicator when related records are not yet fetched and a second has passed', async () => {
const { queryAllByRole } = render(
<ListContextProvider
value={{
Expand All @@ -33,6 +33,8 @@ describe('<ReferenceArrayField />', () => {
</ReferenceArrayFieldView>
</ListContextProvider>
);

await new Promise(resolve => setTimeout(resolve, 1001));
expect(queryAllByRole('progressbar')).toHaveLength(1);
});

Expand Down
2 changes: 1 addition & 1 deletion packages/ra-ui-materialui/src/field/ReferenceArrayField.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
import * as React from 'react';
import { Children, cloneElement, FC, memo, ReactElement } from 'react';
import PropTypes from 'prop-types';
import { LinearProgress } from '@material-ui/core';
import { makeStyles } from '@material-ui/core/styles';
import {
ListContextProvider,
Expand All @@ -16,6 +15,7 @@ import {
import { fieldPropTypes, PublicFieldProps, InjectedFieldProps } from './types';
import { ClassesOverride } from '../types';
import sanitizeFieldRestProps from './sanitizeFieldRestProps';
import { LinearProgress } from '../layout';
Copy link
Member

Choose a reason for hiding this comment

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

useless change

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Nope, we are now using our own LinearProgress instead of MUI


/**
* A container component that fetches records from another resource specified
Expand Down
49 changes: 36 additions & 13 deletions packages/ra-ui-materialui/src/field/ReferenceField.spec.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,20 +9,43 @@ import TextField from './TextField';

describe('<ReferenceField />', () => {
afterEach(cleanup);
const record = { id: 123, postId: 123 };

describe('Progress bar', () => {
it('should display a loader on mount if the reference is not in the store', () => {
it("should not display a loader on mount if the reference is not in the store and a second hasn't passed yet", async () => {
const { queryByRole, container } = renderWithRedux(
<ReferenceField
record={{ id: 123, postId: 123 }}
<ReferenceFieldView
record={record}
resource="comments"
source="postId"
reference="posts"
basePath="/comments"
loaded={false}
loading={true}
>
<TextField source="title" />
</ReferenceField>
</ReferenceFieldView>
);
await new Promise(resolve => setTimeout(resolve, 500));
expect(queryByRole('progressbar')).toBeNull();
const links = container.getElementsByTagName('a');
expect(links).toHaveLength(0);
});
it('should display a loader on mount if the reference is not in the store and a second has passed', async () => {
const { queryByRole, container } = renderWithRedux(
<ReferenceFieldView
record={record}
resource="comments"
source="postId"
reference="posts"
basePath="/comments"
loaded={false}
loading={true}
>
<TextField source="title" />
</ReferenceFieldView>
);
await new Promise(resolve => setTimeout(resolve, 1001));
expect(queryByRole('progressbar')).not.toBeNull();
const links = container.getElementsByTagName('a');
expect(links).toHaveLength(0);
Expand All @@ -32,7 +55,7 @@ describe('<ReferenceField />', () => {
const { queryByRole, container } = renderWithRedux(
<MemoryRouter>
<ReferenceField
record={{ id: 123, postId: 123 }}
record={record}
resource="comments"
source="postId"
reference="posts"
Expand Down Expand Up @@ -67,7 +90,7 @@ describe('<ReferenceField />', () => {
<DataProviderContext.Provider value={dataProvider}>
<MemoryRouter>
<ReferenceField
record={{ id: 123, postId: 123 }}
record={record}
resource="comments"
source="postId"
reference="posts"
Expand Down Expand Up @@ -99,7 +122,7 @@ describe('<ReferenceField />', () => {
// @ts-ignore-line
<DataProviderContext.Provider value={dataProvider}>
<ReferenceField
record={{ id: 123, postId: 123 }}
record={record}
resource="comments"
source="postId"
reference="posts"
Expand All @@ -124,7 +147,7 @@ describe('<ReferenceField />', () => {
// @ts-ignore-line
<DataProviderContext.Provider value={dataProvider}>
<ReferenceField
record={{ id: 123, postId: 123 }}
record={record}
resource="comments"
source="postId"
reference="posts"
Expand Down Expand Up @@ -161,7 +184,7 @@ describe('<ReferenceField />', () => {
const { container, getByText } = renderWithRedux(
<MemoryRouter>
<ReferenceField
record={{ id: 123, postId: 123 }}
record={record}
resource="comments"
source="postId"
reference="posts"
Expand Down Expand Up @@ -197,7 +220,7 @@ describe('<ReferenceField />', () => {
<DataProviderContext.Provider value={dataProvider}>
<MemoryRouter>
<ReferenceField
record={{ id: 123, postId: 123 }}
record={record}
resource="comments"
source="postId"
reference="posts"
Expand All @@ -223,7 +246,7 @@ describe('<ReferenceField />', () => {
// @ts-ignore-line
<DataProviderContext.Provider value={dataProvider}>
<ReferenceField
record={{ id: 123, postId: 123 }}
record={record}
resource="comments"
source="postId"
reference="posts"
Expand All @@ -244,7 +267,7 @@ describe('<ReferenceField />', () => {
const { container } = render(
<MemoryRouter>
<ReferenceFieldView
record={{ id: 123, postId: 123 }}
record={record}
source="postId"
referenceRecord={{ id: 123, title: 'foo' }}
reference="posts"
Expand All @@ -266,7 +289,7 @@ describe('<ReferenceField />', () => {
it('should render no link when resourceLinkPath is not specified', () => {
const { container } = render(
<ReferenceFieldView
record={{ id: 123, fooId: 123 }}
record={record}
source="fooId"
referenceRecord={{ id: 123, title: 'foo' }}
reference="bar"
Expand Down
10 changes: 6 additions & 4 deletions packages/ra-ui-materialui/src/field/ReferenceField.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,11 @@ import {
Record,
} from 'ra-core';

import LinearProgress from '../layout/LinearProgress';
import Link from '../Link';
import sanitizeFieldRestProps from './sanitizeFieldRestProps';
import { PublicFieldProps, fieldPropTypes, InjectedFieldProps } from './types';
import { ClassesOverride } from '../types';
import { LinearProgress } from '../layout';
djhi marked this conversation as resolved.
Show resolved Hide resolved

/**
* Fetch reference record, and delegate rendering to child component.
Expand Down Expand Up @@ -139,20 +139,21 @@ export const NonEmptyReferenceField: FC<Omit<
if (React.Children.count(children) !== 1) {
throw new Error('<ReferenceField> only accepts a single child');
}
const { basePath, resource } = props;
const { basePath, resource, reference } = props;
const resourceLinkPath = getResourceLinkPath({
...props,
resource,
record,
source,
basePath,
});

return (
<ResourceContextProvider value={props.reference}>
<ResourceContextProvider value={reference}>
<PureReferenceFieldView
{...props}
{...useReference({
reference: props.reference,
reference,
id: get(record, source),
})}
resourceLinkPath={resourceLinkPath}
Expand Down Expand Up @@ -194,6 +195,7 @@ export const ReferenceFieldView: FC<ReferenceFieldViewProps> = props => {
...rest
} = props;
const classes = useStyles(props);

if (!loaded) {
return <LinearProgress />;
}
Expand Down
21 changes: 20 additions & 1 deletion packages/ra-ui-materialui/src/input/ReferenceArrayInput.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ describe('<ReferenceArrayInput />', () => {
translate: x => `*${x}*`,
};

it('should render a progress bar if loading is true', () => {
it("should not render a progress bar if loading is true and a second hasn't passed", async () => {
const MyComponent = () => <div>MyComponent</div>;
const { queryByRole, queryByText } = render(
<ReferenceArrayInputView
Expand All @@ -28,6 +28,25 @@ describe('<ReferenceArrayInput />', () => {
<MyComponent />
</ReferenceArrayInputView>
);
await new Promise(resolve => setTimeout(resolve, 250));
expect(queryByRole('progressbar')).toBeNull();
expect(queryByText('MyComponent')).toBeNull();
});

it('should render a progress bar if loading is true and a second has passed', async () => {
const MyComponent = () => <div>MyComponent</div>;
const { queryByRole, queryByText } = render(
<ReferenceArrayInputView
{...{
...defaultProps,
loading: true,
input: {},
}}
>
<MyComponent />
</ReferenceArrayInputView>
);
await new Promise(resolve => setTimeout(resolve, 1001));
expect(queryByRole('progressbar')).not.toBeNull();
expect(queryByText('MyComponent')).toBeNull();
});
Expand Down
2 changes: 1 addition & 1 deletion packages/ra-ui-materialui/src/input/ReferenceArrayInput.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import {
} from 'ra-core';

import sanitizeInputRestProps from './sanitizeInputRestProps';
import LinearProgress from '../layout/LinearProgress';
djhi marked this conversation as resolved.
Show resolved Hide resolved
import { LinearProgress } from '../layout';
import Labeled from './Labeled';
import ReferenceError from './ReferenceError';
import { FieldInputProps, FieldMetaState } from 'react-final-form';
Expand Down
20 changes: 19 additions & 1 deletion packages/ra-ui-materialui/src/input/ReferenceInput.spec.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ describe('<ReferenceInput />', () => {

afterEach(cleanup);

it('should render a LinearProgress if loading is true', () => {
it('should render a LinearProgress if loading is true and a second has passed', async () => {
const { queryByRole } = render(
<ReferenceInputView
{...{
Expand All @@ -72,9 +72,27 @@ describe('<ReferenceInput />', () => {
</ReferenceInputView>
);

await new Promise(resolve => setTimeout(resolve, 1001));
expect(queryByRole('progressbar')).not.toBeNull();
});

it("should not render a LinearProgress if loading is true and a second hasn't passed", async () => {
const { queryByRole } = render(
<ReferenceInputView
{...{
...defaultProps,
input: { value: 1 },
loading: true,
}}
>
<MyComponent />
</ReferenceInputView>
);

await new Promise(resolve => setTimeout(resolve, 250));
expect(queryByRole('progressbar')).toBeNull();
});

it('should not render a LinearProgress if loading is false', () => {
const { queryByRole } = render(
<ReferenceInputView
Expand Down
2 changes: 1 addition & 1 deletion packages/ra-ui-materialui/src/input/ReferenceInput.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,9 @@ import {
} from 'ra-core';

import sanitizeInputRestProps from './sanitizeInputRestProps';
import LinearProgress from '../layout/LinearProgress';
import Labeled from './Labeled';
import ReferenceError from './ReferenceError';
import { LinearProgress } from '../layout';
djhi marked this conversation as resolved.
Show resolved Hide resolved

/**
* An Input component for choosing a reference record. Useful for foreign keys.
Expand Down
20 changes: 16 additions & 4 deletions packages/ra-ui-materialui/src/layout/LinearProgress.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,11 @@
import * as React from 'react';
import Progress from '@material-ui/core/LinearProgress';
import Progress, {
LinearProgressProps as ProgressProps,
} from '@material-ui/core/LinearProgress';
import PropTypes from 'prop-types';
import { makeStyles } from '@material-ui/core/styles';
import classnames from 'classnames';
import { useTimeout } from 'ra-core';

const useStyles = makeStyles(
theme => ({
Expand All @@ -24,18 +27,27 @@ const useStyles = makeStyles(
*
* @param {Object} classes CSS class names
*/
const LinearProgress = props => {
const LinearProgress = ({ timeout = 1000, ...props }: LinearProgressProps) => {
const { classes: classesOverride, className, ...rest } = props;
const classes = useStyles(props);
return (
const oneSecondHasPassed = useTimeout(timeout);

return oneSecondHasPassed ? (
<Progress className={classnames(classes.root, className)} {...rest} />
);
) : null;
};

LinearProgress.propTypes = {
classes: PropTypes.object,
className: PropTypes.string,
timeout: PropTypes.number,
};

// wat? TypeScript looses the displayName if we don't set it explicitly
LinearProgress.displayName = 'LinearProgress';

export interface LinearProgressProps extends ProgressProps {
timeout?: number;
}

export default LinearProgress;
3 changes: 2 additions & 1 deletion packages/ra-ui-materialui/src/layout/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import HideOnScroll, { HideOnScrollProps } from './HideOnScroll';
import Layout, { LayoutProps } from './Layout';
import Loading from './Loading';
import LoadingPage from './LoadingPage';
import LinearProgress from './LinearProgress';
import LinearProgress, { LinearProgressProps } from './LinearProgress';
import LoadingIndicator from './LoadingIndicator';
import Menu, { MenuProps } from './Menu';
import MenuItemLink, { MenuItemLinkProps } from './MenuItemLink';
Expand Down Expand Up @@ -57,6 +57,7 @@ export type {
ErrorProps,
HideOnScrollProps,
LayoutProps,
LinearProgressProps,
MenuItemLinkProps,
MenuProps,
ResponsiveProps,
Expand Down