Join GitHub today
GitHub is home to over 28 million developers working together to host and review code, manage projects, and build software together.
Sign upNested Overlay #51
Conversation
sandstrom
changed the title from
Support nested overlay
to
Nested overlay
Jun 30, 2015
sandstrom
changed the title from
Nested overlay
to
Nested Overlay
Jun 30, 2015
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
sandstrom
Jul 7, 2015
Contributor
@lukemelia @samselikoff friendly ping
We use ember-modal-overlay in our app, and it's great! Though we'd need something along the lines of this PR for some upcoming UI enhancements, and if possible we'd prefer to keep using the mainline instead of a custom fork.
|
@lukemelia @samselikoff friendly ping We use ember-modal-overlay in our app, and it's great! Though we'd need something along the lines of this PR for some upcoming UI enhancements, and if possible we'd prefer to keep using the mainline instead of a custom fork. |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
samselikoff
Jul 7, 2015
Collaborator
I'll try to get to this in the next few days
On Tue, Jul 7, 2015 at 2:33 PM sandstrom notifications@github.com wrote:
@lukemelia https://github.com/lukemelia @samselikoff
https://github.com/samselikoff friendly ping [image:😄 ] I'd love
to hear your thoughts on this!We use ember-modal-overlay in our app, and it's great! Though we'd need
something along the lines of this PR for some upcoming UI enhancements, and
if possible we'd prefer to keep using the mainline instead of a custom fork.—
Reply to this email directly or view it on GitHub
#51 (comment)
.
|
I'll try to get to this in the next few days
|
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
sandstrom
Jul 14, 2015
Contributor
@lukemelia @samselikoff Sorry to prod, just wondering if you've had time to look at this?
|
@lukemelia @samselikoff Sorry to prod, just wondering if you've had time to look at this? |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
samselikoff
Jul 14, 2015
Collaborator
Almost done, switched over an app to your branch yesterday and it seems to be working great! Today I'll know for sure.
|
Almost done, switched over an app to your branch yesterday and it seems to be working great! Today I'll know for sure. |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
|
Works for me! Looks like there are some failing tests |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
chrislopresto
Jul 14, 2015
Contributor
@sandstrom @samselikoff FYI, this needs a rebase.. must have been forked a version or so ago.
|
@sandstrom @samselikoff FYI, this needs a rebase.. must have been forked a version or so ago. |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
sandstrom
Jul 14, 2015
Contributor
@chrislopresto Can you explain more, looking at the commits I don't see why a rebase would be needed (https://github.com/sandstrom/ember-modal-dialog/commits/master). But I'm happy to be corrected if I'm wrong! :)
Regarding the failing tests, I'm not very good at them. Is that something you'd like to assist with?
(I've fixed one issue, which was with == instead of ===)
|
@chrislopresto Can you explain more, looking at the commits I don't see why a rebase would be needed (https://github.com/sandstrom/ember-modal-dialog/commits/master). But I'm happy to be corrected if I'm wrong! :) Regarding the failing tests, I'm not very good at them. Is that something you'd like to assist with? |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
samselikoff
Jul 14, 2015
Collaborator
Looks to me like it's rebased off latest, so you're good there.
Your PR has one more failing test:
not ok 37 PhantomJS 1.9 - component:component-that-uses-modal-dialog: it shows component when open modal clicked
---
actual: >
null
message: >
Died on test #1 at http://localhost:7357/assets/test-support.js:2691
at test (http://localhost:7357/assets/test-support.js:1647)
at http://localhost:7357/assets/dummy.js:2621
at http://localhost:7357/assets/vendor.js:150
at tryFinally (http://localhost:7357/assets/vendor.js:30)
at http://localhost:7357/assets/vendor.js:156
at s (http://localhost:7357/assets/vendor.js:59170)
at s (http://localhost:7357/assets/vendor.js:59170)
at http://localhost:7357/assets/test-loader.js:29
at http://localhost:7357/assets/test-loader.js:21
at http://localhost:7357/assets/test-loader.js:40
at http://localhost:7357/assets/test-support.js:5463: Assertion Failed: A helper named 'ember-modal-dialog-overlay' could not be found
Log: >
...
|
Looks to me like it's rebased off latest, so you're good there. Your PR has one more failing test:
|
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
|
Whoops, don't mind me. :) |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
sandstrom
Jul 14, 2015
Contributor
@chrislopresto I've pushed a couple of iterations, fixing issues in my PR.
However, for the most recent error I don't understand what's wrong (it only seem to affect one of the three modes/branches/scenarios). Any pointers?
not ok 37 PhantomJS 1.9 - component:component-that-uses-modal-dialog: it shows component when open modal clicked
---
actual: >
null
message: >
Assertion after the final `assert.async` was resolved
Log: >
...
|
@chrislopresto I've pushed a couple of iterations, fixing issues in my PR. However, for the most recent error I don't understand what's wrong (it only seem to affect one of the three modes/branches/scenarios). Any pointers?
|
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Keeo
Jul 15, 2015
Well thats awkward (commit e4222da)
ok 36 PhantomJS 2.0 - component:component-that-uses-modal-dialog: it doesn't show component on start
ok 37 PhantomJS 2.0 - component:component-that-uses-modal-dialog: it shows component when open modal clicked
ok 38 PhantomJS 2.0 - JSHint - unit/components: unit/components/component-that-uses-modal-dialog-test.js should pass jshint
1..38
# tests 38
# pass 38
# fail 0
Keeo
commented
Jul 15, 2015
|
Well thats awkward (commit e4222da) ok 36 PhantomJS 2.0 - component:component-that-uses-modal-dialog: it doesn't show component on start
ok 37 PhantomJS 2.0 - component:component-that-uses-modal-dialog: it shows component when open modal clicked
ok 38 PhantomJS 2.0 - JSHint - unit/components: unit/components/component-that-uses-modal-dialog-test.js should pass jshint
1..38
# tests 38
# pass 38
# fail 0 |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
samselikoff
Jul 15, 2015
Collaborator
@Keeo did you run that on 1.13.4? I think that's the version that's failing
|
@Keeo did you run that on 1.13.4? I think that's the version that's failing |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Keeo
Jul 16, 2015
Right, sorry didn't notice that travis is ran across multiple versions. Anyway after more time than I would like to admit I can't get it work either. Moreover it's not really fair to force @sandstorm to fix bug that is related to updated qunit in latest ember stack.
Keeo
commented
Jul 16, 2015
|
Right, sorry didn't notice that travis is ran across multiple versions. Anyway after more time than I would like to admit I can't get it work either. Moreover it's not really fair to force @sandstorm to fix bug that is related to updated qunit in latest ember stack. |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
lukemelia
Jul 16, 2015
Contributor
I asked @krisselden to take a look at this. He suspects a bug in App.reset.
|
I asked @krisselden to take a look at this. He suspects a bug in |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
lukemelia
Jul 16, 2015
Contributor
I can confirm that this failure does not appear to be related to this PR.
|
I can confirm that this failure does not appear to be related to this PR. |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
chrislopresto
Jul 27, 2015
Contributor
@sandstrom Would you mind rebasing against the latest master? Things should go green. Thanks.
|
@sandstrom Would you mind rebasing against the latest master? Things should go green. Thanks. |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
|
@chrislopresto done! |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
|
@chrislopresto friendly ping |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
chrislopresto
Aug 5, 2015
Contributor
@sandstrom Sorry for the delay. I've been meaning to pull this down and wrap my head around the template verbosity. It may make sense to merge as is and plan a breaking change for a separate minor release in the near future.
|
@sandstrom Sorry for the delay. I've been meaning to pull this down and wrap my head around the template verbosity. It may make sense to merge as is and plan a breaking change for a separate minor release in the near future. |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
sandstrom
Aug 5, 2015
Contributor
@chrislopresto I agree. This is intentionally non-breaking, but—as you are saying—I think we should do a breaking one in the near future.
Especially since this new html/css is a super-set of the previous functionality (in respect to being a visual overlay), i.e. one can easily get the same visual look as previously with minor css/html changes. Still, lets keep that in a separate release.
|
@chrislopresto I agree. This is intentionally non-breaking, but—as you are saying—I think we should do a breaking one in the near future. Especially since this new html/css is a super-set of the previous functionality (in respect to being a visual overlay), i.e. one can easily get the same visual look as previously with minor css/html changes. Still, lets keep that in a separate release. |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
sandstrom
Aug 12, 2015
Contributor
@chrislopresto @lukemelia Hey guys, what's the status here? Do you feel ready to pull this?
|
@chrislopresto @lukemelia Hey guys, what's the status here? Do you feel ready to pull this? |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
chrislopresto
Aug 12, 2015
Contributor
@sandstrom I started a branch over the weekend that passes the test suites and greatly simplifies the templates. We are going to make some minor breaking changes in this release in order to simplify the addon.
My next step is to incorporate your approach to enable this scrolling behavior. Since we're willing to make some small breaking changes, there will be no need to add the {{#if nestedOverlay}} guard, so that's helpful. My timeframe goal is to have everything merged and released by the weekend. Your input will be really helpful.
- Are you using ember-tether in your projects that need this scrolling? The reason I ask is that ember-tehter/tether.js appends its element as a child of the body tag, so there is no benefit to nesting within the template. In fact, the desired centered scrolling behavior is possible on master. I threw together this example: https://github.com/yapplabs/ember-modal-dialog/compare/centered-scrolling
- Along those lines, could you please provide an example of the nested scrolling in the dummy app? We would like to start an 'examples' section within the dummy app to ensure that the addon supports important use cases like this going forward.
Sorry for the delay, but I think the end result will be well worth it. Thanks for the help.
|
@sandstrom I started a branch over the weekend that passes the test suites and greatly simplifies the templates. We are going to make some minor breaking changes in this release in order to simplify the addon. My next step is to incorporate your approach to enable this scrolling behavior. Since we're willing to make some small breaking changes, there will be no need to add the
Sorry for the delay, but I think the end result will be well worth it. Thanks for the help. |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
sandstrom
Aug 12, 2015
Contributor
@chrislopresto Sounds good!
- Not using ember-tether.
- I've added CSS in the PR description (#51 (comment))
|
@chrislopresto Sounds good!
|
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
samselikoff
Aug 18, 2015
Collaborator
@chrislopresto Thanks for putting that centered scrolling example together. I checked it out, and it does not replicate the behavior I outlined in issue 11. Do you know if that behavior is possible? I spent some time with tether but I'm not familiar enough to know if it's possible.
|
@chrislopresto Thanks for putting that centered scrolling example together. I checked it out, and it does not replicate the behavior I outlined in issue 11. Do you know if that behavior is possible? I spent some time with tether but I'm not familiar enough to know if it's possible. |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
samselikoff
Aug 18, 2015
Collaborator
In particular I want (1) the normal app content to not scroll, and (2) the modal content to scroll if the mouse is over the overlay but not the modal itself
|
In particular I want (1) the normal app content to not scroll, and (2) the modal content to scroll if the mouse is over the overlay but not the modal itself |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
samselikoff
Aug 18, 2015
Collaborator
I haven't dug into toggleInPlace but that seems to add the modal to the document flow. Is it possible to use that and simultaneously tether the modal to a target, e.g. modal-overlays? This would be great bc then, some of my modals could have the behavior I want, but I could make others that tether to different elements if I want them to be fixed.
|
I haven't dug into |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
chrislopresto
Aug 18, 2015
Contributor
@samselikoff I believe the way forward here is on a separate branch I have going here: https://github.com/yapplabs/ember-modal-dialog/tree/sandstrom-master
On that branch a modal can specify a layout via the positioning property (soon to be renamed). You will probably want to specify positioning='nested'. That will give you a set of nested divs .wrapper > .overlay > .container, which should provide a workable structure for this scrolling behavior.
|
@samselikoff I believe the way forward here is on a separate branch I have going here: https://github.com/yapplabs/ember-modal-dialog/tree/sandstrom-master On that branch a modal can specify a layout via the |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
samselikoff
Aug 18, 2015
Collaborator
@chrislopresto that would be perfect. Then I could also change positioning to fixed or something else, to get the ember-tether goodness?
|
@chrislopresto that would be perfect. Then I could also change |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
chrislopresto
Aug 18, 2015
Contributor
Tether.js (wrapped by ember-tether) appends its content to the body tag. renderInPlace currently overrides usage of tether. So that combo is a no-go.
These sorts of confusing combinations/misleading templates are going away with the new computed layout approach.
|
Tether.js (wrapped by ember-tether) appends its content to the body tag. These sorts of confusing combinations/misleading templates are going away with the new computed layout approach. |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
chrislopresto
Aug 18, 2015
Contributor
From the sounds of it, you probably do not want tether. Instead, we want some divs directly under body, some with a height: 100vh, some with overflow-y: scroll. Since we won't need to reposition the modal, wormhole alone should do the trick.
|
From the sounds of it, you probably do not want tether. Instead, we want some divs directly under body, some with a |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
sandstrom
Aug 18, 2015
Contributor
@samselikoff I've got the layout your gif shows (in #11) working in our app. Ping me on slack/irc and I'll give you the details.
|
@samselikoff I've got the layout your gif shows (in #11) working in our app. Ping me on slack/irc and I'll give you the details. |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
|
Nice. |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
samselikoff
Aug 18, 2015
Collaborator
@chrislopresto that sounds right. I'd love to have an option to toggle on a per-modal basis because I'd like to have tethered modals and fullscreen modals in the same app.
|
@chrislopresto that sounds right. I'd love to have an option to toggle on a per-modal basis because I'd like to have tethered modals and fullscreen modals in the same app. |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
chrislopresto
Aug 19, 2015
Contributor
In the current release you can set useEmberTether=false on individual modals. In the next release we'll have a more intentional and documented api.
|
In the current release you can set |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
ianpetzer
Sep 3, 2015
@chrislopresto @samselikoff I've hoping for very much the same functionality that @samselikoff described with his animated gifs.
Are you hoping to merge the sandstrom-master branch back into master at some point?
I'm just tying to understand if this is something ember-modal-dialog wants to support?
Thanks
ianpetzer
commented
Sep 3, 2015
|
@chrislopresto @samselikoff I've hoping for very much the same functionality that @samselikoff described with his animated gifs. Are you hoping to merge the sandstrom-master branch back into master at some point? I'm just tying to understand if this is something ember-modal-dialog wants to support? Thanks |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
chrislopresto
Sep 4, 2015
Contributor
@ianpetzer Yes, we are aiming to merge #76 shortly. This will provide the structure you need for the scrolling technique @samselikoff described. I believe he has this in a (production?) app based on a fork in this vein.
I just got back from a vacation that delayed some changes by a week, but I'm digging back in tonight.
|
@ianpetzer Yes, we are aiming to merge #76 shortly. This will provide the structure you need for the scrolling technique @samselikoff described. I believe he has this in a (production?) app based on a fork in this vein. I just got back from a vacation that delayed some changes by a week, but I'm digging back in tonight. |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
ianpetzer
Sep 4, 2015
Great news!
Thanks for the update. I'll see how much success I have with the branch in
the interim
On Fri, Sep 4, 2015 at 3:12 AM, Chris LoPresto notifications@github.com
wrote:
@ianpetzer https://github.com/ianpetzer Yes, we are aiming to merge #76
#76 shortly. This
will provide the structure you need for the scrolling technique
@samselikoff https://github.com/samselikoff described. I believe he has
this in a (production?) app based on a fork in this vein.I just got back from a vacation that delayed some changes by a week, but
I'm digging back in tonight.—
Reply to this email directly or view it on GitHub
#51 (comment)
.
ianpetzer
commented
Sep 4, 2015
|
Great news! Thanks for the update. I'll see how much success I have with the branch in On Fri, Sep 4, 2015 at 3:12 AM, Chris LoPresto notifications@github.com
|
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
ianpetzer
Sep 4, 2015
@sandstrom @samselikoff Any advice on how to get the modals working as described in #11 ?
I've pulled in the sandstrom-master branch, copied in the css described at the top of this PR and defined my modal with:
{{#modal-dialog translucentOverlay=true close='backToCountryPage'
clickOutsideToClose=true positioning='nested'}}
However, the scroll is still happening on the page in the background instead of the modal.
Thanks for any suggestions
ianpetzer
commented
Sep 4, 2015
|
@sandstrom @samselikoff Any advice on how to get the modals working as described in #11 ? I've pulled in the sandstrom-master branch, copied in the css described at the top of this PR and defined my modal with:
However, the scroll is still happening on the page in the background instead of the modal. Thanks for any suggestions |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
sandstrom
Sep 4, 2015
Contributor
@ianpetzer With the 'sandstrom-master' plus the example in the dummy app it should work. Relevant files below.
ember-modal-dialog/tests/dummy/app/templates/application.hbs
Lines 200 to 222 in a0e035c
|
@ianpetzer With the 'sandstrom-master' plus the example in the dummy app it should work. Relevant files below. ember-modal-dialog/tests/dummy/app/templates/application.hbs Lines 200 to 222 in a0e035c |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
chrislopresto
Sep 4, 2015
Contributor
@ianpetzer Just a heads up.. I have done a fair bit of work off of the sandstrom-master branch that will serve as the basis for our next release. I'd be happy to chat here or in the EmberJS slack to make sure your new work won't need to be refactored on (let's say) Monday.
|
@ianpetzer Just a heads up.. I have done a fair bit of work off of the sandstrom-master branch that will serve as the basis for our next release. I'd be happy to chat here or in the EmberJS slack to make sure your new work won't need to be refactored on (let's say) Monday. |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
ianpetzer
Sep 4, 2015
@sandstrom Thanks for that.. I've managed to get it up and running although I haven't figured out yet why my modal is shifted to the right instead of centered... Will look into it.
@chrislopresto That would be great. Thanks. Monday would be perfect
ianpetzer
commented
Sep 4, 2015
|
@sandstrom Thanks for that.. I've managed to get it up and running although I haven't figured out yet why my modal is shifted to the right instead of centered... Will look into it. @chrislopresto That would be great. Thanks. Monday would be perfect |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
chrislopresto
Sep 6, 2015
Contributor
@ianpetzer When you have a moment, take a look at #79 It incorporates @sandstrom's work and includes a working "centered scrolling" example from @samselikoff in the modal-dialog portion of the dummy app (at the bottom of the page).
|
@ianpetzer When you have a moment, take a look at #79 It incorporates @sandstrom's work and includes a working "centered scrolling" example from @samselikoff in the modal-dialog portion of the dummy app (at the bottom of the page). |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
ianpetzer
commented
Sep 7, 2015
|
@chrislopresto Thanks this is looking great. Having a play with it now. |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
lukemelia
Sep 10, 2015
Contributor
Closing in favor of #79, which I've just merged and will release momentarily.
|
Closing in favor of #79, which I've just merged and will release momentarily. |
sandstrom commentedJun 30, 2015
Basically does two things:
This allows for the scrolling behaviour outlined in #11. It also enables Holy Grail™ centering of the modal, using the CSS quoted below.
Everything is disabled by default (to avoid breaking existing usage). It's activated via a
nestedOverlay. Unfortunately this makes the template pretty verbose, if this could be considered breaking it would be easier.Closes #11
Please let me know what you think! @lukemelia @samselikoff