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

Added tests and further implementation of paper-dialogs #308

Conversation

mellatone
Copy link
Contributor

Did

  • Refactored paper-wormhole to key off the rootElement
  • Add basic test around dialogs
  • Dialog support for scoping backdrop and container to parent selector
  • Modified docs to highlight dialog scoping
  • Removed alert and confirm examples
  • Key event handling: escape to close

Still outstanding

  • Implement tests and functionality for transitions
  • Button focusing
  • Key event handling: return on focused button should click the button.

@mellatone
Copy link
Contributor Author

I feel like "enter on focused button to trigger action" is best as a {{paper-button}} concern because modals may contain forms that need complex tab indexing. Thoughts: @DanChadwick?

@miguelcobain
Copy link
Collaborator

Can you explain me the reasoning behind removing from contentFor and adding an initializer?

@mellatone
Copy link
Contributor Author

@miguelcobain There's a few reasons I have for suggesting this change. The changes are specifically around refactoring how <div id="paper-wormhole"></div> is set up. If {{content-for}} has other use that I didn't consider, please let me know.

  • It's testable. While hard-coding <div id="paper-wormhole"></div> into the template through the build pipeline works for now, there's an assumption that the wormhole is there. The initializer is testable, and because a lot of other components depend on it, it feels better to make sure that it works.
  • It respects the app's environment. Placing the wormhole underneath the body, assumes that this is where the application lives and where components that use wormhole should be rendered. This may not always be the case. The rootElement is a more accurate representation of that.
  • It better supports embedded applications. One practical example of this is documentation, where we want to demo an embedded application. Wormhole'd components should be rendered relative to this.
  • This technique has precedent. See: https://github.com/yapplabs/ember-modal-dialog/blob/master/addon/initializers/add-modals-container.js
  • Integration testing. Wormhole'd components are being rendered into QUnit's body element, not inside the testing container. For testing components like dialogs, it feels more robust to have the wormhole available each test by explicitly rendering it. (See: https://github.com/miguelcobain/ember-paper/pull/308/files#diff-eb52e3198460432405aa4bacaf342c31R9) Writing tests that assume the wormhole lives in the QUnit body and also accessing QUnit's body feels unreliable.

@miguelcobain
Copy link
Collaborator

@mellatone point given!

@cibernox maybe you're interested?

miguelcobain added a commit that referenced this pull request Mar 15, 2016
Added tests and further implementation of paper-dialogs
@miguelcobain miguelcobain merged commit ab4d880 into adopted-ember-addons:dialogs-jj-abrams Mar 15, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants