Consider all query params in parent routes #97
Conversation
hbrysiewicz
force-pushed
the
respect-query-params
branch
3 times, most recently
from
785f18b to
bff1c81
Compare
There was a problem hiding this comment.
@hbrysiewicz Looks like a conflict that could be resolved, but otherwise I like the concept!
| @@ -44,7 +44,7 @@ export default Mixin.create({ | |||
| } | |||
| const scrollElement = get(this, 'service.scrollElement'); | |||
|
|
|||
| const preserveScrollPosition = get(lastTransition, 'handler.controller.preserveScrollPosition'); | |||
| const preserveScrollPosition = transitions.some((transition) => get(transition, 'handler.controller.preserveScrollPosition')); | |||
There was a problem hiding this comment.
Is it possible to walk backwards on the transitions. Would it make a difference here?
There was a problem hiding this comment.
@hbrysiewicz So if there are 5 transitions do we want to start at the last one and walk backwards until we find one with handler.controller.preserveScrollPosition? This way will start at the earliest one. What are your thoughts?
There was a problem hiding this comment.
Yeah you are right. It's just a boolean value. Ignore my comment!
|
@hbrysiewicz I apologize. I switched to CircleCi and changed linting. If you could get the latest from master and run: yarn
yarn eslintAfter you push this PR should be GTG. ~🙏 |
|
@hbrysiewicz @snewcomer I'm happy to get this branch setup with passing checks. ...I don't want to mess with @hbrysiewicz's awesome work, though. Let me know. 🙏 |
|
@hbrysiewicz I think is is a good addition. Might be best to open another PR at this point, esp with this commit that landed in master. What do you think? |
hbrysiewicz
force-pushed
the
respect-query-params
branch
from
9038f44 to
9d02eff
Compare
|
@snewcomer was able to get this updated |
| @@ -44,7 +44,7 @@ export default Mixin.create({ | |||
| } | |||
| const scrollElement = get(this, 'service.scrollElement'); | |||
|
|
|||
| const preserveScrollPosition = get(lastTransition, 'handler.controller.preserveScrollPosition'); | |||
| const preserveScrollPosition = transitions.some((transition) => get(transition, 'handler.controller.preserveScrollPosition')); | |||
There was a problem hiding this comment.
@hbrysiewicz So if there are 5 transitions do we want to start at the last one and walk backwards until we find one with handler.controller.preserveScrollPosition? This way will start at the earliest one. What are your thoughts?
| @@ -61,7 +61,7 @@ | |||
| "test:all": "ember try:each" | |||
| }, | |||
| "dependencies": { | |||
| "ember-app-scheduler": "^0.2.0", | |||
| "ember-app-scheduler": "kyleshay/ember-app-scheduler#64a0c91d8866b356439bb2d3068029ac59490090", | |||
There was a problem hiding this comment.
Should we try 0.2.2 here? Fyi - ember-app-scheduler is not the default anymore.
There was a problem hiding this comment.
Not sure what you mean "not the default anymore"
Was this dep removed?
There was a problem hiding this comment.
Still there. Just you need to opt into ember-app-scheduler (see README). There was a noticeable jump in the dummy app b/c ember-app-scheduler moves work until after first meaningful paint. To get rid of that jump, we schedule scrollTop work in the render queue.
Lmk if you notice or have any thoughts on that. I could have missed something...
|
@snewcomer - it doesn't matter which controller it is set on, its a query param. So if its in the URL, any controller that has it set should have the same value. |
|
@snewcomer @hbrysiewicz We previously had our own reset-scroll solution, but I was constantly fixing it. So, I just ripped ours out and threw ember-router-scroll in and it works great. This PR fixes our issue very nicely! What is stopping this from being dusted off and merged? (is there anything I can do?) If I understand correctly, from looking at the commits, it seems like only a 1 line diff. |
|
@Duder-onomy Just a rebase + revert change to pkg.json! |
|
Just opened #149 to speed the process. |
Currently only the most recent transition is examined to determine whether to preserve the scroll position. This means that any parent route query parameters are not being respected. This works fine for isolated pages, like the tab example, but does not work when query params exist on the
applicationroute, for example.In my case I dynamically set query parameters on the
applicationroute to show/hide modals. I would expect that when I setpreserveScrollPosition=trueon theapplicationcontroller that even though I'm on a deeply nested sub-route it would be respected. Currently this is not the case.This PR updates the line that examines only the last transition so that it checks all transitions involved to see if any of them have the parameter set.