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

Fixes #37915 - Give Toasts keys for ToastsList #10350

Merged
merged 1 commit into from
Oct 29, 2024

Conversation

MariaAga
Copy link
Member

toastProps key was sometimes empty, so it was overriding the key, I moved the key prop to be after the spread so it wont get overriden but will take the toastProp key if its not empty.
This was causing this error:
react.development.js:315 Warning: Each child in a list should have a unique "key" prop.

Check the render method of ToastsList
in Alert (created by ToastsList)

@adamruzicka
Copy link
Contributor

toastProps key was sometimes empty

Have you found out when and how that happens?

@MariaAga
Copy link
Member Author

Because we dont get them from rails.

  def toast_notifications_data
    selected_toast_notifications = flash.select { |key, _| key != 'inline' }

    selected_toast_notifications.map do |type, notification|
      notification.is_a?(Hash) ? notification : { :type => type, :message => notification }
    end
  end

The key is used for React to differentiate between tags in an rendered list, which is why we had "key={key}", but accidentally override it.

@adamruzicka
Copy link
Contributor

I'm still trying to get my bearings so please bear with me. In case key can be null/undefined, shouldn't we be doing toastProps.key || key everywhere we currently use key?

Also could you please point me to the place where toastProps.key is populated?

@MariaAga
Copy link
Member Author

Thanks for sending me to investigate this, I changed the pr to instead not send an undefined value in the railsMessages, since it will always be undefined and will override the random generated key given in:
webpack/assets/javascripts/react_app/components/ToastsList/slice.js

     const key = toast.key || nanoid();
     return { payload: { key, toast } };

@adamruzicka
Copy link
Contributor

adamruzicka commented Oct 29, 2024

Thanks for sending me to investigate this

That wasn't my intention, I'm just slowly trying to wrap my head around all the frontend bits we have, but thank you for diving into it.

will override the random generated key given in webpack/assets/javascripts/react_app/components/ToastsList/slice.js

Ah, so that's where it comes from

Copy link
Contributor

@adamruzicka adamruzicka left a comment

Choose a reason for hiding this comment

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

LGTM

@adamruzicka adamruzicka merged commit f266ccd into theforeman:develop Oct 29, 2024
59 of 66 checks passed
@adamruzicka
Copy link
Contributor

Thank you @MariaAga !

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

Successfully merging this pull request may close these issues.

2 participants