-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Replaces Flash with react-hot-toast #1856
Conversation
I even helped with some release notes @thedavidprice! |
This will need rewriting: https://redwoodjs.com/docs/flash-messaging-bus |
<FlashMessageComponent key={msg.id} message={msg} timeout={timeout} /> | ||
))} | ||
</div> | ||
const Flash = () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can also use to get design-time warnings in VSCode. It'll be struck through.
/** @deprecated this thing is going to go away. */
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ooo okay I’ll add that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like that only deprecates it in the Redwood package itself, not if you use it in your app? Or am I doing it wrong?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's correct, I'll try to see why it isn't working outside the project.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we still accept an optional timeout
, to keep backwards compatibility with ppl using <Flash timeout={3000} />
in TS projects
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm we could, we'd have to forward that value to the more in-depth version that Toast uses, where you set the timeout per type (success, error, loading...): https://react-hot-toast.com/docs/toaster Although we're already providing a big ol' deprecation notice which should make it pretty clear why your toast may not be showing for the same amount of time it used to (in addition to looking completely different).
Question: how come you called out TS projects here specifically? Is there some different usage from plain JS?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, with the code as it is, in a TS project you'd ge a red squiggly line, saying that doesn't accept a timeout
prop. JS doesn't care if you pass extra stuff, but TS does.
So I'm just suggesting to add it, to make the code compile. Not implement the actual functionality
^^ @dac09 @peterp I'm not sure if there's anything else on a "will-deprecate-in-the-future" list. Either way, maybe now's the time to create one? (PP, I liked the Tailwind reference about this you showed me once, fwiw.) And/or maybe we just deprecate it fully now? RC has already provided really nice messaging with instructions. |
@thedavidprice Yeah I was thinking that we put that message in the release notes as soon this is merged. That gives people the maximum amount of time to switch over before the API disappears completely in v1.0. |
Done! https://github.com/redwoodjs/redwoodjs.com/pull/608 |
@cannikin I've moved these things into |
Test summaryRun details
View run in Cypress Dashboard ➡️ This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard |
dismissMessage(messageId: number): void | ||
cycleMessage(messageId: number): void | ||
/** @deprecated addMessage is deprecated and will be removed in RedwoodJS v1.0. Please update your components to use the `toast()` API: https://react-hot-toast.com/docs/toast */ | ||
const addMessage = (text: string) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above, should we still have options
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Tobbe I don't have enough context to add these, could you help me out?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@peterp sure thing. I'll try to get it done after the kids hava gone to bed tonight
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok - let me chat with @thedavidprice. We might actually just remove Flash completely!
I think I'm going to merge this now, and we can decide what to do when we release, since technically we should have a code freeze today :P
b8b9b10
to
69688e8
Compare
@Tobbe how important are these TS things? Code freeze is today and we were hoping to get this in. I don't even know if I'm fixing any TS inference things, VS Code is so filled with squiggles and yellow underlines and italics and cross-outs I just ignore them all at this point. :( |
@cannikin I was waiting for @peterp to get back, he said he was going to discuss with @thedavidprice about removing the old Flash stuff altogether. Dunno what the status is there |
@Tobbe Although he did say he was just going to merge anyway and then figure out what to do later! He told me earlier today I could merge it, but wasn't sure where your questions about the TS stuff stood... all the checks pass, will bad things happen somewhere if we merge without adding those options? |
@cannikin I haven't had time to verify, but what I think will happen is people with TS projects that use either (or both) of those two options will get compilation errors. Everyone with JS projects will be fine |
Ahhh, okay. Now that I'm looking, the generated |
Yes, if their scaffolded files are .tsx files I just tried this in a project. If this is a .js file, everything works
But if I rename it to .tsx I get this The good thing though is that it actually runs. And no errors in the web browser. This was better than I feared. It will say they're deprecated, so maybe this is good enough? I keep forgetting that we're actually not typechecking before building. We do that at work, so I'm so used to nothing running if there are TS errors. But with a RW project it's actually fine-ish. |
Oh, that's not so bad! Yeah I can live with that. Can I merge? |
Do it! |
Did it! |
Closes #1839, #1701
This replaces our custom Flash stuff with
react-hot-toast
and updates the scaffold generators to generate with the new interface. It will output deprecation notices to the browser console letting users know to migrate and that the old API will be gone as of v1.0.Docs Update
Docs have been updated to go along with this PR, merge https://github.com/redwoodjs/redwoodjs.com/pull/608 after this PR is released!
Release Notes
Unless you have a super-custom implementation of messaging (where you're using
dismissMessage()
andcycleMessage()
manually) you actually won't notice any difference, other than the messages will look different. You'll get a deprecation in the Web Inspector letting you know that're dropping<Flash>
andaddMessage()
in v1.0 and to switch over to using<Toaster>
andtoast()
.If you are using
dismissMessage()
andcycleMessage()
they will now be a noop and you'll just see the deprecation notice.Updating from Flash to Toaster
This change is fairly easy. Import
Toaster
and replace instances of<Flash>
with<Toaster>
:Toaster
accepts some additional options thatFlash
did not, including the position of the popup on the page, the order of new messages, and more. See them on the official react-hot-toast docs.Updating from addMessage() to toast()
Note that since
toast
is not a hook you can use it outside of a component (even outside of React itself, according to the docs!).toast()
takes some addition options which you can read about in their docs.Updating Scaffold Styles
If you have
web/src/scaffold.css
for styling your scaffolds, there's one rule you'll want to change if you want the toast's icons to display properly: