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

Add <SingleFieldList empty gap direction> props, and allow it to be used without children #9439

Merged
merged 4 commits into from
Nov 14, 2023

Conversation

fzaninotto
Copy link
Member

@fzaninotto fzaninotto commented Nov 10, 2023

Problems

<SingleFieldList> has many issues:

  • It still clones its children (we don't do this since 3.x)
  • It forces users to pass children, while most of the time it's a <ChipField>
  • It renders kind of a Stack, except it's not a Stack
  • It doesn't support the empty prop

Solution

  • <SingleFieldList> now renders a real <Stack>, and supports all its props
  • <SingleFieldList> used without children renders a <ChipField> with the recordRepresentation
  • <SingleFieldList> supports an empty prop
const PostList = () => (
    <List>
        <Datagrid>
            <TextField source="id" />
            <TextField source="title" />
            <DateField source="published_at" />
            <BooleanField source="commentable" />
            <NumberField source="views" />
            <ReferenceArrayField label="Tags" reference="tags" source="tags">
                <SingleFieldList />
            </ReferenceArrayField>
        </Datagrid>
    </List>
)

Closes #7634

Minor BC Break

If you used <SingleFieldList> with a <ChipField> child and enabled the links, the chips background will no longer change on hover. To reenable this visual effect, add the clickable prop to the <ChipField>:

const PostList = () => (
    <List>
        <Datagrid>
            <TextField source="id" />
            <TextField source="title" />
            <DateField source="published_at" />
            <BooleanField source="commentable" />
            <NumberField source="views" />
            <ReferenceArrayField label="Tags" reference="tags" source="tags">
                <SingleFieldList>
-                  <ChipField source="name" />
+                  <ChipField source="name" clickable />
               </SingleFieldList>
            </ReferenceArrayField>
        </Datagrid>
    </List>
)

Alternately, if the <ChipField source> is the same as the resource's recordRepresentation, you can omit the child altogether.

const PostList = () => (
    <List>
        <Datagrid>
            <TextField source="id" />
            <TextField source="title" />
            <DateField source="published_at" />
            <BooleanField source="commentable" />
            <NumberField source="views" />
            <ReferenceArrayField label="Tags" reference="tags" source="tags">
-               <SingleFieldList>
-                  <ChipField source="name" />
-              </SingleFieldList>
+              <SingleFieldList />
            </ReferenceArrayField>
        </Datagrid>
    </List>
)

@fzaninotto fzaninotto added the RFR Ready For Review label Nov 10, 2023
Comment on lines -92 to -97
{cloneElement(Children.only(children), {
record,
resource,
// Workaround to force ChipField to be clickable
onClick: handleClick,
})}
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 is technically a BC break, as people may use SingleFieldList with children expecting record as prop. However, it's not supposed to be the case since 4.0.

@@ -174,7 +181,11 @@ const Root = styled('div', {
// useful to prevent click bubbling in a datagrid with rowClick
const stopPropagation = e => e.stopPropagation();

// Our handleClick does nothing as we wrap the children inside a Link but it is
Copy link
Member Author

Choose a reason for hiding this comment

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

We no longer need this: the default child uses the clickable prop only when necessary. And when developers pass a custom child, they'll have to set the clickable themselves if they want. In that case, this PR is a slight BC break.

Copy link
Contributor

Choose a reason for hiding this comment

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

Unless I'm mistaken, until now developers always had to pass a child. So it's a BC for everyone using <SingleFieldList> right now unless they have specifically disabled the link.

I agree it's a very minor BC, but it will still impact almost everyone using <SingleFieldList> right now. Maybe we should insist on it more in the docs and/or the changelog?

Copy link
Member Author

Choose a reason for hiding this comment

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

You're right. I updated the PR description, and we should include the "Minor BC break" section in the release changelog.

linkType = 'edit',
component: Component = Root,
Copy link
Member Author

Choose a reason for hiding this comment

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

component is now passed down to <Stack>, which knows how to handle it.

@fzaninotto fzaninotto changed the title Add <SingleFieldList empty> prop to customize empty value Update <SingleFieldList> to make it more powerful Nov 10, 2023
@adguernier adguernier self-requested a review November 10, 2023 15:33
Copy link
Contributor

@slax57 slax57 left a comment

Choose a reason for hiding this comment

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

Other than that, it works great! 🎉
And the stories are very helpful.

@@ -174,7 +181,11 @@ const Root = styled('div', {
// useful to prevent click bubbling in a datagrid with rowClick
const stopPropagation = e => e.stopPropagation();

// Our handleClick does nothing as we wrap the children inside a Link but it is
Copy link
Contributor

Choose a reason for hiding this comment

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

Unless I'm mistaken, until now developers always had to pass a child. So it's a BC for everyone using <SingleFieldList> right now unless they have specifically disabled the link.

I agree it's a very minor BC, but it will still impact almost everyone using <SingleFieldList> right now. Maybe we should insist on it more in the docs and/or the changelog?

@slax57 slax57 added this to the 4.16.0 milestone Nov 14, 2023
@slax57 slax57 merged commit 9768ee1 into next Nov 14, 2023
@slax57 slax57 deleted the singlefieldlist-empty branch November 14, 2023 10:21
@fzaninotto fzaninotto changed the title Update <SingleFieldList> to make it more powerful Add <SingleFieldList empty gap direction> props, and allow it to be used without children Nov 16, 2023
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.

3 participants