Merge lp://staging/~rockstar/phazr/close-overlay into lp://staging/phazr
Proposed by
Paul Hummer
| Status: | Merged |
|---|---|
| Approved by: | Paul Hummer |
| Approved revision: | 9 |
| Merged at revision: | 4 |
| Proposed branch: | lp://staging/~rockstar/phazr/close-overlay |
| Merge into: | lp://staging/phazr |
| Diff against target: |
148 lines (+103/-1) 5 files modified
examples/modalplugin/index.html (+36/-0) src/css/phazr.css (+4/-0) src/js/modalplugin/modalplugin.js (+36/-0) tests/index.html (+5/-1) tests/modalplugin.js (+22/-0) |
| To merge this branch: | bzr merge lp://staging/~rockstar/phazr/close-overlay |
| Related bugs: |
| Reviewer | Review Type | Date Requested | Status |
|---|---|---|---|
| Deryck Hodge | code | Approve | |
|
Review via email:
|
|||
Commit message
Add a close button to overlays
Description of the change
This branch adds a plugin that will add a close button to an overlay, as well as a keylistener that will hide the overlay if "Esc" is hit.
The one problem with this branch currently is that close is text, not an icon, like I'd like it to be. The same problem exists with Save/Cancel on the FormPlugin stuff. I plan on remedying that as my next task.
As a modal dialog, might it also make sense that we make the overlay draggable as well? I couldn't decide, and it's not really that much harder to do. I just wasn't sure, so I'd like to hear your input on it.
To post a comment you must log in.
Preview Diff
[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
Looks generally good, Paul.
I do think the overlay should be dragable. Always thought that actually about overlays, so completely agree with you. I would also like to be able to click away from the overlay, and have it dismiss. It's a nice feature to have when people do bad overlays and blow up the screen, hiding the close button. It's not always obvious ESC is your friend in those situations.
Also, the test isn't wired up to the test runner index.html. The module code is, but not the test code. If I do wire it up, it errors. So I think you need to look into something there. I supposed the test page title could be something other than "Form Plugin" too.
The example runs fine and looks good, though. And the code looks good to me, too. I'll mark this approved, assuming you'll fix up the test. And let you toggle the MP approved when it's ready.