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

create a Dialog base type #166

Closed
pixelzoom opened this issue Sep 29, 2014 · 29 comments
Closed

create a Dialog base type #166

pixelzoom opened this issue Sep 29, 2014 · 29 comments
Assignees

Comments

@pixelzoom
Copy link
Contributor

As noted in #154, AboutDialog is not generalizable into a Dialog subtype. So we're going to start over. Design and implement joist.Dialog, to be used as the base type for AboutDialog, OptionDialog, and other future dialogs.

@pixelzoom
Copy link
Contributor Author

Proposed responsibilities:

Dialog responsibilities:
• dialog is a container for a 'content' node
• dialog optionally responds to resizing of its content
• dialog may be modal or non-modal
• non-modal dialog must be closed via close button or some interaction with dialog’s component
• modal dialog grays out everything behind it, prevents interaction with anything but the dialog (including navbar), clicking outside of it closes it

PhET menu responsibilities (global dialogs):
• creates modal dialogs exclusively
• adds a dialog to the current screen, removes it from the current screen when dismissed
• responsible for positioning of the dialog (typically centered on screen)

Screen responsibilities (screen-specific dialogs):
• a screen can create a modal or non-modal dialog, and is responsible for adding/removing it from its ScreenView
• responsible for positioning of the dialog (typically centered on screen)

Proposed constructor interface and options:

  /**
   * @param {Node} content
   * @param {Object} [options]
   * @constructor
   */
  function Dialog( content, options ) {

    options = _.extend( {
      modal: false, // {boolean} modal dialogs prevent interaction with the rest of the sim while open
      title: null, // {Node} title to be displayed at top
      titleAlign: 'center', // horizontal alignment of the title: {string} left, right or center
      hasCloseButton: true, // whether to put a close 'X' button is upper-right corner
      cornerRadius: 10, // {number} radius of the dialog's corners
      resize: true, // {boolean} whether to resize if content's size changes
      fill: 'white', // {string|Color}
      stroke: 'black' // {string|Color}
    }. options );

    Node.call( this );

    // implementation goes here ...

    this.mutate( options );
  }

@samreid
Copy link
Member

samreid commented Sep 29, 2014

clicking outside of it closes it

I believe at a recent design meeting, a "x" close button was also requested.

EDIT: I noticed you already said: • non-modal dialog must be closed via close button or some interaction with dialog’s component

@pixelzoom
Copy link
Contributor Author

From proposed options:

hasCloseButton: true, // whether to put a close 'X' button is upper-right corner

@samreid
Copy link
Member

samreid commented Sep 29, 2014

Other issues to discuss:

  • The Dialog should scale up/down as the screen size changes (is this guaranteed by being a child of the ScreenView? Will this work properly for the home screen?)
  • Should the simulation continue to animate in the background while a model dialog is shown?

@pixelzoom
Copy link
Contributor Author

The Dialog should scale up/down as the screen size changes (is this guaranteed by being a child of the ScreenView? Will this work properly for the home screen?)

That is the hope. And I don't see why it wouldn't. If it doesn't for the home screen, then it's a flaw in the home screen. (HomeScreen does extend ScreenView, so hopeful here.)

Should the simulation continue to animate in the background while a model dialog is shown?

That is up to the client, not a responsibility of Dialog. In the case of the PhetMenu client, the current behavior is that animation continues while the About dialog is open. That can certainly be changed, but it's a change to PhetMenu.

@jonathanolson
Copy link
Contributor

A very common use-case will be to have it centered in the available counts. I'd like to not have that code duplicated, even if it doesn't live with Dialog (as its level of abstraction seems perfect).

@pixelzoom
Copy link
Contributor Author

Agreed, there should be an easy way to center both modal and non-modal dialog

@pixelzoom
Copy link
Contributor Author

Looks like theres a dialog in Neuron that is based on AboutDialog. Will need to deal with that.

@jonathanolson has another type of dialog in Molecule Shapes (see Molecule3DDialog).

@jbphet has possibly another type of dialog in Balancing Act (see MassValueEntryNode).

@samreid
Copy link
Member

samreid commented Oct 3, 2014

I wonder if any of the solution the we build for AboutDialog would be applicable to the PhET popup menu, which currently exhibits problems like: #115. Aren't popup menus somewhat dialog-ish? Same question for ComboBox popup.

@samreid
Copy link
Member

samreid commented Oct 8, 2014

Please see #73 for discussion about the bounds implementation issues.

@pixelzoom
Copy link
Contributor Author

Reassigning to @jonathanolson since he has a more immediate need for Options dialog.

jonathanolson added a commit that referenced this issue Oct 8, 2014
…xed the layout issues with the PhetMenu (fixes #114, fixes #115)
@jonathanolson
Copy link
Contributor

Unsure of how we'll want close buttons to look, and how to lay out things generally with a close button. Implementation will need tweaking and review.

@jonathanolson
Copy link
Contributor

Additionally, implementation is currently leaking listeners on the sim's resize when the dialog is closed.

jonathanolson added a commit that referenced this issue Jan 29, 2015
…tioned correctly in the corner, and doesn't push content over), see #154, phetsims/molecule-shapes#96, #166
@pixelzoom
Copy link
Contributor Author

Moving this from #154 ... The title and close button need some work, they occupy too much vertical space. For example, here's the Options dialog from Molecule Polarity:

61a360d8-a79d-11e4-8041-0cfa28a8673d

@pixelzoom
Copy link
Contributor Author

At 1/29/15 dev meeting, there was also discussion about trying this style of close button, like this example from least-squares-regression. (This may make centering problematic.)

screenshot_444

@pixelzoom
Copy link
Contributor Author

@ariel-phet and @pixelzoom will triage the feature requirements.

@pixelzoom pixelzoom assigned ariel-phet and unassigned jonathanolson Feb 5, 2015
@samreid
Copy link
Member

samreid commented Mar 30, 2015

I'm going to need a semitransparent overlay for phetsims/scenery#414 Can @pixelzoom and/or @jonathanolson comment on whether this feature should use the barrierRectangle used by dialogs, or something new and separate?

@pixelzoom
Copy link
Contributor Author

If the barrier rectangle is decoupled from Dialog, then it's probably appropriate to use it. If it's not decoupled from Dialog, but would fit the bill, then decouple it. Not recommended to create something new and identical to the barrier rectangle.

@samreid
Copy link
Member

samreid commented Mar 30, 2015

Here's what the code looks like at the moment:

    // Semi-transparent black barrier used to block input events when a dialog (or other popup) is present, and fade
    // out the background.
    this.barrierStack = new ObservableArray();
    this.barrierRectangle = new Rectangle( 0, 0, 1, 1, 0, 0, {
      fill: 'rgba(0,0,0,0.3)',
      pickable: true
    } );
    this.topLayer.addChild( this.barrierRectangle );
    this.barrierStack.lengthProperty.link( function( numBarriers ) {
      sim.barrierRectangle.visible = numBarriers > 0;
    } );
    this.barrierRectangle.addInputListener( new ButtonListener( {
      fire: function( event ) {
        assert && assert( sim.barrierStack.length > 0 );
        sim.hidePopup( sim.barrierStack.get( sim.barrierStack.length - 1 ), true );
      }
    } ) );

My main question is whether we have modal dialog support yet. The overlay rectangle is conceptually similar to a (invisible) modal dialog.

@pixelzoom
Copy link
Contributor Author

I don't know if we have modal dialog support yet. I've been somewhat out of the loop on Dialog development.

@jonathanolson
Copy link
Contributor

The barrier rectangle is decoupled, however it won't block all input events as desired. I've implemented display.interactive (phetsims/scenery#414) which should fit the bill for blocking events.

@samreid
Copy link
Member

samreid commented Mar 31, 2015

display.interactive is working perfectly for my purposes, thanks. I may still use the barrier rect to show the sim as grayed out though, see #232

@pixelzoom
Copy link
Contributor Author

In 8/16/15 dev meeting agenda, @ariel-phet asked:

Should this issue be revisited? Closed?

I'll review, to see what still needs to be done.

@pixelzoom
Copy link
Contributor Author

I created issues (see above) for some of the things that haven't been completed.

A more general issue is that responsibilities have not been implemented as proposed in the requirements. From #166 (comment):

PhET menu responsibilities (global dialogs):
• creates modal dialogs exclusively
• adds a dialog to the current screen, removes it from the current screen when dismissed
• responsible for positioning of the dialog (typically centered on screen)

Screen responsibilities (screen-specific dialogs):
• a screen can create a modal or non-modal dialog, and is responsible for adding/removing it from its ScreenView
• responsible for positioning of the dialog (typically centered on screen)

Deviations:

• Screen and ScreenView are never involved.
• Sim handles show/hide.
• Dialog (not Sim or Screen) handles positioning, and it's limited to centering.

Assigning to @jonathanolson to comment on why it's desirable to have Sim (which already has too many responsibilities, see #208) handle show/hide. And why are we limited to centering?

@pixelzoom pixelzoom assigned jonathanolson and unassigned pixelzoom Aug 19, 2015
@pixelzoom
Copy link
Contributor Author

Also wondering how the current implementation will handle non-modal dialogs, where the user may change screens while one or more dialogs (specific to the current screen) are displayed.

@pixelzoom
Copy link
Contributor Author

I created a joist demo application for testing Dialog, see joist/joist_en.html. Numerous problems noted with non-modal dialogs, see #293.

@jonathanolson
Copy link
Contributor

Assigning to @jonathanolson to comment on why it's desirable to have Sim (which already has too many responsibilities, see #208) handle show/hide. And why are we limited to centering?

It doesn't have to be sim, but it has to be something with access to the root node (which Sim has), and probably something global (since the background put up behind needs to be tracked in once place, and it needs to try to "exit" the top dialog if allowable).

And it doesn't necessarily need to be centered, is there a good use-case for non-centered?

@pixelzoom
Copy link
Contributor Author

is there a good use-case for non-centered?

Any non-modal dialog. (And any API I've ever worked with that supports dialogs.)

@jbphet
Copy link
Contributor

jbphet commented Aug 20, 2015

@pixelzoom has reviewed and has created specific issues for the problems described above. This was reviewed at the 8/20/2015 dev meeting and we agreed it can be closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants