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

Simplify the development of custom List views #4952

Merged
merged 36 commits into from
Jun 30, 2020
Merged

Simplify the development of custom List views #4952

merged 36 commits into from
Jun 30, 2020

Conversation

fzaninotto
Copy link
Member

@fzaninotto fzaninotto commented Jun 17, 2020

Problem

  1. For the core team, passing around list controller props is cumbersome, and makes TypeScript types bloated and cryptic.
  2. It's hard to know which variables are provided by react-admin in a list
  3. As a consequence, it's hard to develop a custom List layout (e.g. with metrics at the top, or with both a left and a right sidebar)

Solution

Use a context to store controller props instead of cloning children and passing them the prop.

Continue cloning the child and passing controller props for now, to keep BC, but ignore these props in child components.

Target

No need to "push" the props from parent to child, the children can "pull" the props from the context.

const PostList = props => (
    <BaseList {...props} perPage={10}>
        <div>
             <Filter />
        </div>
        <Grid container>
            <Grid item xs={8}>
                <SimpleList primaryText={record => record.title} />
            </Grid>
            <Grid item xs={4}>
                List instructions...
            </Grid>
        </Grid>
        <div>
            Post related links...
        </div>
    </BaseList>
);

Tasks

  • Add ListContext and useListContext to store and read list controller variables
  • Add <ListBase> to simplify the combination of <ListContext> and useListController
  • Migrate <List> and <ListView> to TypeScript
  • Make List create a <ListContext.Provider>
  • Turn <ListView> into a top/right/bottom/left/center layout (do the layout you want with ListBase)
  • use useListContext instead of props in Pagination
  • use useListContext instead of props in Filter
  • use useListContext instead of props in Empty
  • use useListContextinstead of props in BulkActionsToolbar
  • use useListContext instead of props in ListToolbar
  • use useListContext instead of props in Datagrid
  • use useListContext instead of props in SingleFieldList
  • use useListContext instead of props in SimpleList
  • use useListContext instead of props in ArrayField
  • use useListContext instead of props in ReferenceManyField (and add filtering capabilities)
  • use useListContext instead of props in ReferenceArrayField (and add sorting, pagination and filtering capabilities)
  • use useListContext instead of props in ListGuesser
  • Add tests
  • Add documentation (basically rewrite the List documentation)

Slight BC breaks:

  • useListController no longer returns the version
  • useReferenceManyFieldController returns a basePath instead of a referenceBasePath

@fzaninotto fzaninotto added the WIP Work In Progress label Jun 17, 2020
Adds filtering capabilities to the ReferenceManyField

Slight BC breaks:
- useListController no longer returns the version
- useReferenceManyFieldController returns a basePath instead of a referenceBasePath
} = props;
if (React.Children.count(children) !== 1) {
throw new Error(
'<ReferenceManyField> only accepts a single child (like <Datagrid>)'
);
}
const { sort, setSortField } = useSortState(initialSort);
Copy link
Member Author

Choose a reason for hiding this comment

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

this logic has moved to useReferenceManyFieldController

Copy link
Contributor

@djhi djhi left a comment

Choose a reason for hiding this comment

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

Pretty neat! Several things should have been done in dedicated PRs though (Datagrid conversion to TS, Reference fields migration to ListContext)

packages/ra-core/src/controller/useListContext.ts Outdated Show resolved Hide resolved
fzaninotto and others added 12 commits June 19, 2020 16:07
And demonstrate how to do a custom layout in TagList
- use List Context in ExportButton, ListActions, Filter
- introduce ListBase
- merge permanentFilter with Filter in ListParams to avoid having to do it several times and passing the permanent filters around
- use ListBase in TagList and CommentList
Copy link
Contributor

@djhi djhi left a comment

Choose a reason for hiding this comment

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

💪

packages/ra-ui-materialui/src/list/List.spec.js Outdated Show resolved Hide resolved
packages/ra-ui-materialui/src/list/List.spec.js Outdated Show resolved Hide resolved
@fzaninotto fzaninotto added RFR Ready For Review and removed WIP Work In Progress labels Jun 28, 2020
@fzaninotto
Copy link
Member Author

Now ready for review!

docs/List.md Outdated Show resolved Hide resolved
@@ -965,15 +963,15 @@ By default, the filter form doesn't provide a submit button, and submits automat

React-admin doesn't provide any component for that, but it's a good opportunity to illustrate the internals of the `<Filter>` component. We'll actually provide an alternative implementation to `<Filter>`.

As explained earlier, `<List>` clones the `filters` element twice. It passes special props to the element to let it interact with the URI query parameter more easily:
As explained earlier, `<List>` clones the `filters` element twice - once to display the filter button, and once to display the filter form. This `filters` element can use the `useListContext` hook to interact with the URI query parameter more easily. the hook returns the following constants:
Copy link
Contributor

Choose a reason for hiding this comment

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

constants?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, these are constants, not variables or props. How would you name them?

Copy link
Contributor

Choose a reason for hiding this comment

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

I found it confusing because it returns functions as well

docs/List.md Outdated Show resolved Hide resolved
docs/List.md Outdated Show resolved Hide resolved
Co-authored-by: Gildas Garcia <1122076+djhi@users.noreply.github.com>
Copy link
Contributor

@djhi djhi left a comment

Choose a reason for hiding this comment

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

💪

Copy link

@ThieryMichel ThieryMichel left a comment

Choose a reason for hiding this comment

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

The ramda part is optional :)

}, [setSelectedIds]);

// filter logic
const [displayedFilters, setDisplayedFilters] = useSafeSetState<{

Choose a reason for hiding this comment

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

Doesn't the useFilterState hook already allow to do that ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Unfortunately, this hook doesn't handle the complex logic of setting filters, displayed filters, and page at the same time, so I can't use it in this case.

if (!loaded) return;
let finalData = data;
// 1. filter
Object.keys(filterValues).forEach(

Choose a reason for hiding this comment

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

I think it would be better to start form the data, with a data.filter that would then check all filter values.
This way no need to duplicate data.

Additionally, you could use ramda whereEq to filter data
https://ramdajs.com/docs/#whereEq
I checked, ramda is already in react-admin dependency (well in yarn.lock actually)

Copy link
Member Author

Choose a reason for hiding this comment

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

TIL ramda.wherEq

Yes, it would prevent iterating several times over the data for multiple filters. BUT Ramda isn't a direct dependency, it's significantly harder to code, and I doubt many people will download very large arrays and do multicriteria search on them. And when it's the case, we'll investigate your optimization.

Copy link

@ThieryMichel ThieryMichel Jun 30, 2020

Choose a reason for hiding this comment

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

Not that hard to code:

const tempData = data.filter(record =>
    Object.entries(filterValues)
        .all(([filterName, filterValue]) => filterValue == get(record, filterName)),
);

Or even better

const matchFilter = filterValues => record => Object.entries(filterValues)
        .all(([filterName, filterValue]) => filterValue == get(record, filterName));

// later
const tempData = data.filter(matchFilter(filterValues));

And easier to understand IMO

);
// 2. sort
if (sort.field) {
finalData = finalData.sort((a, b) => {

Choose a reason for hiding this comment

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

Ramda has a sortBy function https://ramdajs.com/docs/#sortBy
Just saying :)

}, [setSelectedIds]);

// filter logic
const [displayedFilters, setDisplayedFilters] = useSafeSetState<{

Choose a reason for hiding this comment

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

useFilterState ?

Copy link
Member Author

@fzaninotto fzaninotto Jun 29, 2020

Choose a reason for hiding this comment

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

Unfortunately, this hook doesn't handle the complex logic of setting filters, displayed filters, and page at the same time, so I can't use it in this case.

if (!loaded) return;
let finalData = data;
// 1. filter
Object.keys(filterValues).forEach(
Copy link

@ThieryMichel ThieryMichel Jun 30, 2020

Choose a reason for hiding this comment

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

Not that hard to code:

const tempData = data.filter(record =>
    Object.entries(filterValues)
        .all(([filterName, filterValue]) => filterValue == get(record, filterName)),
);

Or even better

const matchFilter = filterValues => record => Object.entries(filterValues)
        .all(([filterName, filterValue]) => filterValue == get(record, filterName));

// later
const tempData = data.filter(matchFilter(filterValues));

And easier to understand IMO

@djhi djhi merged commit efd3959 into next Jun 30, 2020
@djhi djhi deleted the controller-context branch June 30, 2020 12:28
@fzaninotto fzaninotto added this to the 3.7.0 milestone Jun 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
RFR Ready For Review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants