-
Notifications
You must be signed in to change notification settings - Fork 1
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
playing around with flashes #83
Conversation
// ============================================================ | ||
|
||
function transitionEnd() { | ||
var el = document.createElement('bootstrap') |
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.
Missing semicolon.
@jrubenoff: didya get a chance to check this out? |
This is pretty slick. Added some refinements... let me know if you have any questions re: my comments. I also added some nicer colors for the flashes. |
Thanks for the comments, I learned a lot! I never would have thought to move the entire I think it looks pretty great too, but the one thing that bugs me about the current transition is that it looks like the Star Wars credits. Maybe if it animated less, or was a little more subtle? If you want to merge this soon, we could, or if you want to wait until you do your larger refactor of our components, that might make sense too. |
I don't really see the "Star Wars credits" thing, but removing the scale transform will do the trick if it's bothering you. |
Also, I'm down for merging this in whenever. |
Alright, a few reasons this isn't ready... but it's gonna be awesome. I removed the scale transform and it's much better... I also added I added link styles, but they look kinda shoddy: The current behavior is that alerts don't hide when they're hovered over, but with the new Should we remove that behavior and add the "persist alerts that have actions" (#88) behavior instead? Or should we fix that behavior? |
Yeah, I would like to merge this because it's a good first step for turning flashes into a more ambient notifications system like Atom's... more prominent, but easier to ignore when you need to. |
Yeah, I want to merge too, but I still don't know how to implement persistent flashes.
In #88 you mention:
But I'm not sure what you mean by an "action". Maybe when step 2 happens in the process above, we just start stacking up the non-persisted flashes until the persistent flash is dismissed? This might actually be Atom's behavior: https://cloud.githubusercontent.com/assets/69169/5176406/350d0e80-73fd-11e4-8101-1776b9d6d8bf.gif (This is when I start to wonder if we're getting too complex.) |
So, currently, I think the answer is simple: Flash 1 hides when 2 appears. We don't have passive notifications, so flashes will always be caused by the user deliberately doing something. In the future, maybe we want to add passive notifications which have nothing to do with the current user's activity, like:
Etc. In that case, we would need to make this more complex. |
....So then shouldn't we just hide any notification when another one On Wed, May 20, 2015 at 1:58 PM, Josh Rubenoff notifications@github.com
Adam Becker |
...yes, but like the current behavior, we need to give each flash enough time on the screen to be read, which means they might stack if they appear one after the other? |
Alright, let me try developing this and then we can reason about it more. On Wed, May 20, 2015 at 2:23 PM, Josh Rubenoff notifications@github.com
Adam Becker |
@jrubenoff: take a look at the current implementation. The code is prototype-quality, but I'm wondering if this behavior works? Another potential problem, though: we're getting rid of the "close" link, which means that if a persistent flash is on top of e.g. a nav item, there's no way to click that nav item. What should we do about this? |
I've gotten this branch to a place that I'm 👍 on shipping, which is a simplified version of what we've been working on, with:
But I'd like us to figure this out, first:
|
Ideas:
|
Just merged-in master. Getting this figured out would make me really happy. |
How do I test persistent flashes? They're not in the style guide. Let's add a close button only for persistent flashes. |
Without looking at the code:
On Mon, Sep 21, 2015 at 7:01 PM, Josh Rubenoff notifications@github.com
Adam Becker |
Not sure why PhantomJS is missing in the tests above, but this looks good! I cleaned up the styles a bit and updated from master. I encountered the bug below a few times, where a persistent flash appears multiple times, but I have no idea to reproduce: (Even the GIF above was made by pressing the trigger link in rapid succession, so... I have no idea what I did to cause the actual bug.) |
I'm seeing that bug too. I also don't really dig the location of the flashes on the page. If they're not going to be centered, I think they should be even further towards the top-right corner. Right now, they look like they're floating in no-man's land. I think the best thing to do is figure out what we like about this approach, and then I'll try and re-implement off of Here's what I think we've concluded on:
(Anything I'm missing?) Here's what we still need to decide:
|
Also: added icons to help colorblind users better distinguish between different types of flashes. |
968d2ce
to
79f1b65
Compare
Just did this for fun... and because I wanted to learn a bit more about CSS transitions. Here's a quick screencast: http://take.ms/wNWbD
.flash_close
button, I have literally never clicked it beforeI couldn't get the CSS transition to look totally 👌. I'd be super interested to learn how you do it, @jrubenoff.
Also, is this a direction that you'd want to build in?