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

we broke Scaffold useFlash #1839

Closed
thedavidprice opened this issue Feb 23, 2021 · 9 comments · Fixed by #1856
Closed

we broke Scaffold useFlash #1839

thedavidprice opened this issue Feb 23, 2021 · 9 comments · Fixed by #1856
Labels
bug/confirmed We have confirmed this is a bug help wanted

Comments

@thedavidprice
Copy link
Contributor

For Scaffolded UI, we include useFlash for all CRUD. However, Flash messages no longer appear for each case:

  • Create (NewPost.js) = ⛔
  • Read = NA
  • Update (EditPostCell.js) = ⛔
  • Delete (Post.js) = ⛔
  • Delete (Posts.js) = ✅

The Contact Submit Flash message is working correctly.

My suspicion is that we broke this sometime during the Apollo v3 upgrade process.

Video Demo of missing Flash messaging

Screen.Recording.2021-02-22.at.5.38.48.PM.mov
@thedavidprice thedavidprice added help wanted bug/confirmed We have confirmed this is a bug labels Feb 23, 2021
@cannikin
Copy link
Member

Maybe related to #1701

We kind of came to the conclusion to replace ours with https://react-hot-toast.com/

@thedavidprice
Copy link
Contributor Author

'cause Hot Toast is the best. name. ever.

@BurnedChris
Copy link
Contributor

We have been using react-hot-toast as a replacement for use flash and its been splendid.

@cannikin
Copy link
Member

While we're up for breaking things, I'm thinking I'll just use the real API from react-hot-toast rather than trying to proxy everything through our stuff like useFlash and addMessage. Should we have people import from react-hot-toast or do we import/export ourselves in @redwoodjs/web so you still import from there?

If I can figure out how to do it, I'll forward messages from addMessage to toast but output a big warning to the console to let people know that they should be using the real toast API and ours will be going away as of v1.0.

@cannikin
Copy link
Member

@peterp How do you feel about adding react-hot-toast to web? Or should we make it a separate package like forms to save package sizes for those that end up not using it? Looks like the compiled size of the production, gzipped minified bundle is 3.5kb. 11kb before gzip.

@peterp
Copy link
Contributor

peterp commented Feb 25, 2021

I'm using it on Snaplet as well and it's awesome!

If I can figure out how to do it, I'll forward messages from addMessage to toast but output a big warning to the console to let people know that they should be using the real toast API and ours will be going away as of v1.0.

Sounds like a plan!

How do you feel about adding react-hot-toast to web? Or should we make it a separate package like forms to save package sizes for those that end up not using it? Looks like the compiled size of the production, gzipped minified bundle is 3.5kb. 11kb before gzip.

I think it's OK to add it now, our own package is 10kb, and we can always remove it at a later stage.

@BurnedChris
Copy link
Contributor

Maybe it would just be best to cut it out and just recommend it in a cookbook, as then redwood does not need to worry about wrapping it and keeping it up to date! @peterp @cannikin

@cannikin
Copy link
Member

We do want something nice in the scaffolds though...

I was planning on just importing * and exporting everything back out in @redwoodjs/web so you would use the native interface. I was just going to wrap for pre-v1.0 so people that are currently using <Flash> aren't screwed.

@BurnedChris
Copy link
Contributor

BurnedChris commented Feb 25, 2021

I think opt in could work if we deffo need something in the scaffolds. Like forms so if you wanted to just use it by its self there would not be a problem, or you could use the wrapper if wanted!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug/confirmed We have confirmed this is a bug help wanted
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants