Feature: Confirm Dialog's content prop can receive React.ReactNode

[andrico1234]

Makes the Confirm component more flexible while retaining existing behaviour, i.e. if a string is passed through, the content is translated, otherwise simply render the component.

The video demonstrates this behaviour in action.

content-is-react-delement.mov

Note: While I updated the all of the uses of Content within the React Admin codebase, please let me know if there's anything else I need to consider.

I'll also be happy adding a cypress test too.

[andrico1234]

I'm not handling this conditional logic in a very elegant way. I wasn't sure how best you'd like this done. The two solutions I have in mind are

1. Leave it as it is

2. Do somethng like this

import MUIContent from '@material-ui/blahblah`

function Content(props) {
  if (typeof props.content === 'string') {
    return (
      <DialogContentText>
        {translate(content, {
          _: content,
         ...translateOptions,
        })}
    </DialogContentText>
  }

  return props.content
}

function Confirm() {
  
  return (
    <Dialog {..existingProps}> 
      {otherElements}
      <Content />
      {otherElements}
    </Dialog>
  )
}

Or any other suggestions you folks might have

[andrico1234]

I also didn't make a ticket for this, so I'm happy if you don't believe this fits with the repo

[djhi]

Hey and thanks for contributing. If think it would be easier to accept an optional children prop which would take precedence over the content prop when provided, wouldn'it?

[andrico1234]

I would be concerned that this would increase the component's complexity, and increase the cognitive overhead. e.g.

• What's the difference between content and children?
• Why is the value of content not being displayed? (when a user uses both props)

It also increases potential errors and gotchas. While we can make use of discriminated union types (i think) to ensure only one of the two props are passed through, it won't stop JavaScript users from getting themselves into this broken state.

What do you think?

[djhi]

I think the difference is that:

• you only have to check for the presence of each prop and choose one over the other which looks simpler to me that type guards
• it's closer to React semantics

Besides, I would even deprecate the content prop in favor of the children actually, keeping it only until v4.

[andrico1234]

I agree that replacing content with children feels like a more natural API. I'd be happy to to make this change, and add a deprecation warning if you folks agree that it's the right thing to do

One question that I have is how to handle translating the text in this case. It would like look we'd still need type guards. If it's a string, pass the value through the translate function

[djhi]

I think it's for the best, yes. Please make sure the warning is only displayed in development mode though. Thanks for taking the time to do this!

[djhi]

One question that I have is how to handle translating the text in this case. It would like look we'd still need type guards. If it's a string, pass the value through the translate function

Right, missed that one... We may have to keep both for a while, maybe providing a <Translated key="..." /> component you could pass as the children. Let me check with the other core team members and get back to you

[fzaninotto]

Hi, and thanks for your contribution.

I don't understand the use case here. Can you describe why an end user would want to see something else than text there?

Also, I don't want to change the way such a simple component behaves in v4, so I don't think we should add a deprecation warning. Let's keep that for important changes.

[andrico1234]

One of the things I manage the chat thread. Each thread is a resource, and has its own Show component. In the Show's the Action bar we have a button that lets us close the thread. On click, it opens up a confirm dialog and asks the user whether they want to proceed closing the thread. In this dialog, we also want to provide a text input to let the user enter a closing message from within the confirm modal.

[fzaninotto]

Interesting. I don't think that's what the generic <Confirm> component was designed for, though. So I tend to think this change doesn't belong in react-admin core but in user land. @djhi what's your take on this?

[andrico1234]

I'm not sure if this is something to consider, but if RA makes heavy use of Material UI to the point where having a basic understanding of MUI is beneficial to using RA, then it would make sense to try and keep the component interfaces similar.

What I mean to say is in MUI's Dialog Component, the client can pass through children as the main content. For me at least, this seems more intuitive than having a content prop that only takes a string. It takes advantage of the IoC pattern where RA doesn't have to worry about the content a user wants to render, and lets the user handle all of that.

The main challenge here is ensuring translation still works as expected

[fzaninotto]

This change is benign and can help in some cases, so let's merge it.

[fzaninotto]

Thanks!

[andrico1234]

Amazing! I only noticed this went in as I read the release notes 😅