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

ReactNode vs string for message #525

Closed
1 task
polewskm opened this issue Feb 19, 2023 · 4 comments
Closed
1 task

ReactNode vs string for message #525

polewskm opened this issue Feb 19, 2023 · 4 comments
Assignees

Comments

@polewskm
Copy link

Description

Would it be possible for the message property to be of type ReactNode vs string?

Explanation / motivation

This would allow any React content to be displayed in the notification vs strings or strings with embedded HTML.

Additional information

  • I can implement this feature/improvement
@LouisBarranqueiro
Copy link
Owner

👋 @polewskm , sorry for the very late reply. Could you please share a concrete example of what you are trying to do? Why string or HTML string do not cover your needs?

Would it be possible for the message property to be of type ReactNode vs string?

It is not supported by default but you can adapt the library to create a notification (Notification) with a given a React component (e.g: Notification.messageComponent), and then you would create your own React Notification to display such notifications.

@polewskm
Copy link
Author

Current use-case with message as string

import React from 'react'
import {useNotifications} from 'reapop'

const AComponent = () => {
    // 1. Retrieve the action to create/update a notification.
    const {notify} = useNotifications()
    
    useEffect(() => {
        // 2. Create a notification.
        notify('Welcome to the documentation', 'info')
    }, [])

    return (
        ...
    )
}

Proposed use-case with message as ReactNode

import React from 'react'
import {useNotifications} from 'reapop'

const AComponent = () => {
    // 1. Retrieve the action to create/update a notification.
    const {notify} = useNotifications()
    
    useEffect(() => {
        // 2. Create a notification.
        notify(<div class='root'><p>Welcome to the documentation</p><p>other content...</p></div>, 'info')
    }, [])

    return (
        ...
    )
}

The purpose of this is to use native react rendering for HTML content vs unsafe rendering of HTML raw strings via the dangerouslySetInnerHTML property that you are currently doing.

@LouisBarranqueiro
Copy link
Owner

LouisBarranqueiro commented Aug 12, 2023

The purpose of this is to use native react rendering for HTML content vs unsafe rendering of HTML raw strings via the dangerouslySetInnerHTML property that you are currently doing.

<div class='root'><p>Welcome to the documentation</p><p>other content...</p></div> is JSX and converted to a React element by your JS compiler like babel. React Elements are not serializable, and therefore can't be put stored in Redux. While we could still hack something to make it work for simple components, an HTML string will do the work perfectly. There is nothing wrong with dangerouslySetInnerHTML attribute as long as you know how to use it. That's why the attribute exists. However, it is marked as dangerous to remind developers that it could be in certain situations and make sure they don't pass any data to it.

The purpose of this library is to propose a simple, yet powerful API but most importantly stable to add notifications to your application. A message as a string fulfills all the use cases this library is built for.

I would suggest building your own custom Notification components and add extra properties to notifications to control/display customized information instead of attempting to store non-serializable (React elements) data in the state of React/Redux.

What these custom notifications would display? In the example you shared, you can replace it with an HTML string but I guess you want to build something more complex with logic into it, right?

@LouisBarranqueiro
Copy link
Owner

@polewskm I'm closing this inactive issue. Please re-open it if needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants