adobe-photoshop / spaces-design

Adobe Photoshop Design Space
http://adobe-photoshop.github.io/
Other
852 stars 74 forks source link

First Launch Dialog rough-in #979

Closed DesignSpaceBot closed 9 years ago

DesignSpaceBot commented 9 years ago

This PR adds a "first launch" modal dialog presented after the application has loaded. It can be permanently dismissed which sets a preference to NOT display at each launch. The current content is just stubbed-out at this point. It includes a barebones "Carousel" component that allows simple navigation through an array of static content items. (Navigate with arrow keys, mouse clicks on the carousel item)

A "Help" Component was used to mount the first launch dialog to the main window. The thought is that it could house future help components that need a place to live.

A new menu item "Open Introduction" will show the first launch dialog at any time.

stuff

The Dialog is modal (make use of the showModal() function supported by CEF on the <dialog> element). The "backdrop" is shaded out, and any click in that region will dismiss the dialog. Other ways to dismiss: hit ESCAPE key.

For the existing non-modal dialogs, the element is still supplied the open property directly instead of using the analogous dialog.open() function. The latter was causing an issue in the post-render processing of Dialog children because they were rendering inside the hidden dialog before it was opened. Namely, the Blend Modes DataList scrolls to the selected value, and that seems to require the DOM to be visible. Anyway, this could be a sticky point going forward for modals.

Changes along the way

Dialog now supports a property which may contain a list of keys that will dismiss the Dialog. These shortcuts are added/removed during open/close/unmount. The ColorPicker has been retrofitted to use this instead of doing the escape key (fixes #487 for free)

Dialogs can now be "registered" prior to being opened. This allows the dismissal policy to be specified in React, but the dialog opened from a flux action (via menu, or onStartup)

-- @mcilroyc at 2015-03-31T17:09:42Z

DesignSpaceBot commented 9 years ago

After a first quick glance, this seems totally sane. Carry on.

-- @iwehrman at 2015-04-01T01:33:59Z

DesignSpaceBot commented 9 years ago

Didn't look through the code, but if I resize the app window, the dialog disappears. That might be undesired.

-- @volfied at 2015-04-01T15:19:24Z

DesignSpaceBot commented 9 years ago

Working with this more: We're gonna want to have some sort of transition between the slides, I'll find out what the desired transition is.

-- @designedbyscience at 2015-04-01T15:50:49Z

DesignSpaceBot commented 9 years ago

Presumably we can/should arrange for those transitions to be implemented in CSS?

-- @iwehrman at 2015-04-01T15:52:57Z

DesignSpaceBot commented 9 years ago

Yea, but that has some impact on how the slides are rendered from React (we'll need some/all of them in the DOM at once)

-- @designedbyscience at 2015-04-01T15:53:48Z

DesignSpaceBot commented 9 years ago

make it so

-- @iwehrman at 2015-04-01T16:06:09Z

DesignSpaceBot commented 9 years ago

I guess this? http://f.cl.ly/items/233B0Y1o2h3D0x37232M/image-removed.png

-- @mcilroyc at 2015-04-01T16:21:28Z

DesignSpaceBot commented 9 years ago

Welllll, that's definitely an option. Another option is to just render all the pages of the carousel and add/remove classes for the active/inactive pages.

-- @iwehrman at 2015-04-01T17:51:25Z

DesignSpaceBot commented 9 years ago

I did a quick mockup using React's CSSTransitionGroup for the animations. 71eae4c0ea798609e9d7ee5eff4ec248acacd518 It doesn't look great, but I also didn't mess with the CSS other than to just copy the example from the docs. So, I don't know if there is a way for @designedbyscience to work some magic to make this look good. BUT, if this doesn't work out, we can revert this back and just do the add/remove classes idea from @iwehrman if it will improve things.

-- @mcilroyc at 2015-04-01T19:25:24Z

DesignSpaceBot commented 9 years ago

Looking at this now

-- @designedbyscience at 2015-04-01T19:33:00Z

DesignSpaceBot commented 9 years ago

nice @designedbyscience ! the transitions look way better. I noticed things can get screwy when you try and arrow too fast, so I'll check in to that. Debouncing for starters.

Otherwise I need to look at disabling the menus, and you/phil are still working out the "how to close it permanently?" question.

-- @mcilroyc at 2015-04-01T21:31:53Z

DesignSpaceBot commented 9 years ago

BTW, I screwed up and left that "merge" commit slip through. I'll fix that in post.

-- @mcilroyc at 2015-04-01T21:32:56Z

DesignSpaceBot commented 9 years ago

Yea, I'm thinking we may just assume the user doesn't want to see it again (especially if it is available via Help)

-- @designedbyscience at 2015-04-01T21:35:26Z

DesignSpaceBot commented 9 years ago

It's interesting (in a bad way) that mouse events seem to be leaking through the modal backdrop somehow to PS. You can pan the canvas with the modal dialog open, for example. It's possible that some crazy policy stuff will need to be adjusted here. Lemme know @mcilroyc if the fix is not obvious.

-- @iwehrman at 2015-04-02T01:47:48Z

DesignSpaceBot commented 9 years ago

Also, if there isn't a good way to do this in CSS, the Dialog will probably need to listen for "resize" events to reposition itself when in "center" positioning mode.

-- @iwehrman at 2015-04-02T01:49:35Z

DesignSpaceBot commented 9 years ago

Armchair designer opinions: the sliding transitions don't feel good to me as is. Also, you should probably be able to click the left and right sides of the dialog to rewind/advance slides.

-- @iwehrman at 2015-04-02T01:50:55Z

DesignSpaceBot commented 9 years ago
2015-04-01 18:56:19.214fluxcontroller.js:273 Executing action shapes.setFillColor after waiting 0ms; 1/0
2015-04-01 18:56:19.236Dialog.jsx:280 Warning: You are calling cloneWithProps() on a child with a ref. This is dangerous because you're creating a new child which will not be added as a ref to its parent.
2015-04-01 18:56:19.253fluxcontroller.js:290 Finished action shapes.setFillColor in 39ms with RTT 39ms; 1/0

-- @iwehrman at 2015-04-02T01:57:28Z

DesignSpaceBot commented 9 years ago

dear @iwehrman,

  1. panning through modal dialog: CNR
  2. resizing: meh, but OK. @designedbyscience thoughts?
  3. transition don't feel right because you can't click left/right? Or are these two separate issues?
  4. Will fix. I think this won't be an issue in v13 version of cloneElement

-- @mcilroyc at 2015-04-02T13:28:32Z

DesignSpaceBot commented 9 years ago
  1. This is the trackpad-scroll gesture. Presumably the same thing happens with a mousewheel scroll.
  2. Yes, resizing!
  3. That the transition doesn't feel right is unrelated to the problem of needing to click left/right hot zones to page. I think the default hypothesis should be no transition, and we should have to agree that some transition is an improvement over no transition. There are almost no animations in the app currently.
  4. Cool. I think we're gonna wait until after May to upgrade to React 0.13 though because of the other scary-sounding async-setState changes.

-- @iwehrman at 2015-04-02T14:14:56Z

DesignSpaceBot commented 9 years ago
  1. Easiest way to handle this is fixed position to the window, position: fixed; left: 50%; top: 50%; transform: translate(-50%,-50%) Then you should get resize centering for free.
  2. It is easy enough to turn off that transition, but no transition is jarring and practically unseen these days. The standard for this sort of layout/pagination module is a sliding transition, though we could look at others (cross fading). One of the reasons that the sliding helps establish place and relationships between the different slide (which is critical when the paging). We can adjust speed/easing to make it better, I did not spend much time tweaking that. Adding scrolling would also make this better (the best would be scrolling to snap, similar to swipe to snap on touch devices) I also think that if we have click left/right to move prev/next, then the sliding helps reinforce that interaction. See http://www.idangero.us/swiper/demos/02-responsive.html for a nice version

If we wanted to do the scrolling thing, it might make more sense to do the overflow: hidden masking technique rather than using React Transition Group. Render all the slides horizontally in a really wide carousel, the frame is overflow:hidden, then use either scrollLeft or translate to change the current one. If we use translate, we can easily use CSS to manage the easing equation.

-- @designedbyscience at 2015-04-02T14:47:41Z

DesignSpaceBot commented 9 years ago

I just pushed a commit that uses eric's suggestion for CSS positioning. Resizing the window now adjusts the modal centering appropriately.

The bad news is that I am still fighting that clone warning from react. more to come on that.

I'm going to recommend that we not make any further significant changes to the carousel transition/interaction in this pull request.

-- @mcilroyc at 2015-04-02T16:23:59Z

DesignSpaceBot commented 9 years ago

rebased with a moderate commit trim

-- @mcilroyc at 2015-04-02T18:17:17Z

DesignSpaceBot commented 9 years ago

Alright gents, click-to-navigate exists now. I just used an even 50/50 split of the carousel div.

On last item that I remembered: right now there is no way to dismiss the dialog permanently. Are we planning to add a button that will explicitly dismiss the dialog? Or should any dismissal be treated as permanent (seems extreme)? @designedbyscience

-- @mcilroyc at 2015-04-02T19:33:31Z

DesignSpaceBot commented 9 years ago

I think it is extreme but acceptable (I mean, isn't that what everyone wants 90% of the time? I've seen complaints about how Hello hides that option past a scroll), but I'm getting second opinions

-- @designedbyscience at 2015-04-02T19:47:37Z

DesignSpaceBot commented 9 years ago

@pluadobe seconds that.

-- @designedbyscience at 2015-04-02T20:13:58Z

DesignSpaceBot commented 9 years ago

Any dismissal should be treated as permanent.

-- @iwehrman at 2015-04-02T20:25:11Z

DesignSpaceBot commented 9 years ago

An so it shall be 5949a2a I'm switching this out of 'review only' mode.

-- @mcilroyc at 2015-04-02T20:44:35Z

DesignSpaceBot commented 9 years ago

I realize this is going to be controversial, but I don't think the carousel should "roll over" from last to first when navigating forward. That is especially true with the current sliding transitions.

-- @iwehrman at 2015-04-06T16:26:28Z

DesignSpaceBot commented 9 years ago

Why?

-- @designedbyscience at 2015-04-06T16:27:08Z

DesignSpaceBot commented 9 years ago

OK, done with a review pass. I have a few gripes and questions, but overall it's looking great. Really lovin' the Dialog cleanup. Back to @mcilroyc.

-- @iwehrman at 2015-04-06T16:38:20Z

DesignSpaceBot commented 9 years ago

@designedbyscience: Why? Because it's not actually an image carousel, and because there is a definitive end. Consequently, I think it makes sense for paging to actually stop at the end. Also, I find the sliding transition disorienting when switching from last to first.

-- @iwehrman at 2015-04-06T17:08:22Z

DesignSpaceBot commented 9 years ago

With that said, I'm not going to fall on my sword for this. But even though in some cases I do think it makes sense to wrap around, I do not think it does for this use case.

-- @iwehrman at 2015-04-06T17:09:00Z

DesignSpaceBot commented 9 years ago

@iwehrman Agreed!

-- @designedbyscience at 2015-04-06T17:09:05Z

DesignSpaceBot commented 9 years ago

@iwehrman @designedbyscience - I will update the Carousel to wrap based on a supplied property, and will update the First Launch to not wrap. Should clicking (or hitting right-arrow) just be a no-op if we're at the end of the list?

-- @mcilroyc at 2015-04-06T19:03:50Z

DesignSpaceBot commented 9 years ago

Correct, no-op if at end (or beginning for left arrow)

-- @designedbyscience at 2015-04-06T19:12:25Z

DesignSpaceBot commented 9 years ago

Alright, assigning this back to @iwehrman. I have a follow-up question about mutable/immutable, and a mild push-back/answer for the "register on the fly, IFF a dismissal policy was provided" thing. Otherwise, the nits have been addressed, and the carousel no longer wraps (for first launch)

-- @mcilroyc at 2015-04-06T20:46:41Z

DesignSpaceBot commented 9 years ago

I don't have any complaints about the on-the-fly registration, I was just wondering. I would like to clean up the immutable->mutable reference thing, but it doesn't have to be right now. Would you either a) propose some solution to that potential problem in the form of an additional commit; or b) file a code-cleanup bug about it?

-- @iwehrman at 2015-04-06T21:53:22Z

DesignSpaceBot commented 9 years ago

@iwehrman, I choose option a. See latest commit. Seem OK? I considered defining internal Immutable.Record types, but went with map-only for now.

-- @mcilroyc at 2015-04-06T23:12:33Z

DesignSpaceBot commented 9 years ago

Darned if I can find anything else to complain about.

-- @iwehrman at 2015-04-07T03:25:22Z