Closed ajb closed 9 years ago
@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 .wrapper
up...
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 translateZ(0)
which makes it smoother.
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 .wrapper
animation, this gets janky: http://take.ms/7M8Gv
Should we remove that behavior and add the "persist alerts that have actions" (#88) behavior instead? Or should we fix that behavior?
Also, I just pushed a change to use inline-block
, which looks muuuuuch better IMO:
Should we remove that behavior and add the "persist alerts that have actions" (#88) behavior instead?
Yes, please
Challenging my original assumptions: do we ever really need to show > 1 flash at once?
Ha, I thought you knew something I didn't.
This is still nice to have, if we ever do need them.
Yeah, that's what I'm trying to balance... nice-to-have vs. goddamn-added-complexity-that-makes-our-lives-more-painful.
I'm also trying to take cues from https://github.com/atom/notifications
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 https://github.com/dobtco/dvl-core/issues/88 you mention:
the flash should stay onscreen until the user performs another action
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 appears?
On Wed, May 20, 2015 at 1:58 PM, Josh Rubenoff notifications@github.com wrote:
So, currently, I think the answer is simple: Flash #1 https://github.com/dobtco/dvl-core/issues/1 hides when #2 https://github.com/dobtco/dvl-core/issues/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:
- "Max Ophuls just invited Robert Bresson to Web Design RFP"
- "Barack Obama submitted a response"
Etc. In that case, we would need to make this more complex.
— Reply to this email directly or view it on GitHub https://github.com/dobtco/dvl-core/pull/83#issuecomment-103974987.
Adam Becker (951) 9-BECKER @AdamJacobBecker
...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 wrote:
...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?
— Reply to this email directly or view it on GitHub https://github.com/dobtco/dvl-core/pull/83#issuecomment-103985932.
Adam Becker (951) 9-BECKER @AdamJacobBecker
@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 :+1: on shipping, which is a simplified version of what we've been working on, with:
display: inline-block
)But I'd like us to figure this out, first:
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?
Ideas:
Other ideas:
From Gmail:
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:
DvlFlash('info', 'blah', true)
On Mon, Sep 21, 2015 at 7:01 PM, Josh Rubenoff notifications@github.com wrote:
How do I test persistent flashes? They're not in the style guide.
Let's add a close button only for persistent flashes.
— Reply to this email directly or view it on GitHub https://github.com/dobtco/dvl-core/pull/83#issuecomment-142132561.
Adam Becker (951) 9-BECKER @AdamJacobBecker
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 master
to see if I can make it less buggy.
Here's what I think we've concluded on:
(Anything I'm missing?)
Here's what we still need to decide:
Not really sure where we're at (and that's OK), but just popping in to say that I'm a big fan of Google Docs' flash:
It persists for about 10 seconds, which gives the user more than enough time to read and react to it, and the "Dismiss" and "Undo" links, while a bit cramped, are pretty clean.
I moved flashes to the bottom right corner of the screen, which grounds them a little better. We could add a close button to all flashes (including those which aren't persistent) if we're worried about overlapping important content.
Mockup in Sketch with new icons:
Also: added icons to help colorblind users better distinguish between different types of flashes.
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 :ok_hand:. I'd be super interested to learn how you do it, @jrubenoff.
Also, is this a direction that you'd want to build in?