-
Notifications
You must be signed in to change notification settings - Fork 970
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
Add flash message component #654
Conversation
Holy cow that was quick! Is this ready for review? Any chance you could put together a little sample app that uses the functionality and put it on Netlify somewhere so we could play with it in action? |
I'd be delighted. :) |
@cannikin I've put together a little demo of the functionality for "intercom" by building a flash component in an example redwood app. When first visiting the site you should see a "persisted" message (top of page). This might be an example of a "We use cookies" message or the like—one that needs to be dismissed via user interaction. If you dismiss this message and want to see it again you'll need to delete the localStorage record I created to mock this behavior. In the admin section (/user) you'll see typical flash messages after creating, updating and deleting user records. These follow a more typical scenario where a message has a lifecycle. It can be dismissed immediately by the user or (in this case) it is dismissed on a route change. It was my intention when developing the Intercom functionality to let the developer make decisions about message lifecycle and UI on the redwood-app side. But we could certainly include a Flash component in the scaffold files to serve as an example, if you and the team decide you like this feature and want to merge it in. Of course, I'm also happy to make any changes you see fit. Thanks for considering! |
Amazing! Thanks so much for the demo app, it makes it really clear how everything works. I'd love get this added to the scaffold generator so you get messaging when creating/editing/deleting records. Is there an easy to way to add a user-defined timeout that lets the message dismiss itself? So if you don't do anything, it'll just close itself after 5 seconds, for example? I always wished the Rails flash had that functionality! One maybe bug we noticed—if you don't dismiss the homepage one and then go to the admin, then back home, it seems to dismiss itself. I found one where I didn't dismiss the homepage one, then created a user in the admin and it showed both messages (as expected). But I didn't dismiss either one, went back to the homepage, and now the original message was gone. Thanks again, this is great! |
Ahhh just looked at the implementation and it looks like that's on purpose! It only shows the banner the first time you visit the site (really the CoreLayout) so any time you leave and come back to that layout it's going to not show the banner again (like when going between the admin and back to the homepage). |
Absolutely.
Yep. I'll add that feature to the Flash component.
Right. That's more or less a "bug" or quirk of my specific implementation in the demo. My thinking is that it's the developers responsibility to code up the rules for when and how a message is dismissed and, hopefully, I've provided enough tooling in the IntercomContext api to make that easy. I'm sure we'll soon find ways that it isn't :) So, cool. Next step is to build a standardized Flash component for the scaffold generator. I'll get started. |
@cannikin Wanted to know what you think—I'm not married to the "Intercom" domain. I was just thinking that it would be cool to name it something different than the rails-y canonical, "Flash", but... If you have an idea or just want to go with "Flash" as it's a recognizable feature name, that's totally fine with me. |
Hmm, my only hesitation with "intercom" is that I always associate that name with https://www.intercom.com/ but maybe that's just me. I'm totally fine with just "Flash", that might even help some folks recognize what it is that know it from the Rails world! |
@cannikin - I've got updated code and a new demo for the changes we discussed here. Code is here.
In the meantime are there any style changes you'd like to see? Do you think we should animate the message in/out in some way? Any further functionality suggestions? Thanks! |
Awesome! I'll take a look this week! |
fixup! add intercom messaging component message.viewed can be set on add change test description rename intercom to flash add basic flash component fixup! add flash message context and component add Flash component to scaffolds and scaffold tests add flash readme Add way to pretty print errors. Format errors using youch. Add onException to watcher. Handle build time errors. Remove timestamp from dev command. Add a way to handle runtime errors in http. Format errors in GraphQL Server. upgrade prisma beta.8 Audit CLI commands Adds positional arguments and descriptions to arguments/options. This means that builder is now usually a function instead of an obejct. Other notes: - desc/describe -> description - adds terminal-link to makde terminal links - almost all commands get an .epilogue, linking to the CLI Reference - down/up have a new pos. arg: decrement/increment - modify dbCommands test for builder now being functions isolate CI runner Windows OS and Node v12 adds ubuntu back to runner Uniform windows/posix "Find me in" path in generated files Fix Page tests on Windows Windows fixes: helpers.test.js Windows fix: component.test.ts (normalize paths) page.test.js: Fix eslint error Windows path fix: cell.test.js Windows path fix: function.test.js Windows path fix: layout.test.js Windows path fix: scaffold.test.js Windows path fix: scaffoldPath.test.js Windows path fix: scaffoldPathMultiword.test.js Windows path fix: scaffoldPathNested.test.js Windows path fix: sdl.test.js Windows path fix: service.test.js add inline explanation add node 14 and windows to Action Runner upgrade apollo-server-lambda per security alert Add support for CSS/SCSS modules Add sourcemaps Add sourcemap support to global scss, css Add more helpful classnames fix linter violation Fix windows matching paths in babel. v0.9.0 upgrade Prisma alpha.1311; yarn.lock Revert "upgrade Prisma alpha.1311; yarn.lock" This reverts commit 3c128e8. Keep .css rules at index:5 Try to ignore tagged versions. v0.9.1 upgrade Prisma alpha.1311; yarn.lock force canary upgrade prisma v2 🎉 v0.10.0 delete Form README this doc was moved to redwoodjs.com repo https://github.com/redwoodjs/redwoodjs.com/blob/master/docs/form.md fixup! add flash readme
@Terris this is looking great! And I ❤️ seeing the doc already included. The "home" for this doc will be the redwoodjs.com repo in the docs/ directory: I can move it after this is merged unless you beat me to it. Just didn't want you to think it disappeared. |
MERGEDDDD Thanks so much for this @Terris! |
I'll take care of the doc merge as well -- aiming for v0.11.0 in the next 18 hours 😉 |
@Terris one last question here. I'm getting the following ESLint warning. Does this look like something to address with a fix, or something we should have ESLint ignore? /redwoodjs-redwood/packages/web/src/flash/Flash.js
10:6 warning React Hook useEffect has missing dependencies: 'cycleMessage' and 'message.id'. Either include them or remove the dependency array react-hooks/exhaustive-deps
18:6 warning React Hook useEffect has missing dependencies: 'dismissMessage', 'message.id', and 'timeout'. Either include them or remove the dependency array react-hooks/exhaustive-deps
/redwoodjs-redwood/packages/web/src/flash/__tests__/flash.test.js
23:8 warning React Hook useEffect has a missing dependency: 'addMessage'. Either include it or remove the dependency array |
This PR is a feature prototype - it adds a messaging component called "Flash".
Usage Demo App
addMessage
,dismissMessage
, andcycleMessage
useIntercom
hook to make the actions and message list readily available to any child of<RedwoodProvider>
An example use for this feature could be to "flash" a message when a record is created, deleted or modified.
MERGE INSTRUCTIONS: