dockyard/ember-router-scroll

🗔 Scroll to top with preserved browser history scroll position.

ember-router-scroll's Issues

Demo site and dummy app

It took me a while to figure this out, but it looks like there are two demo sites for this addon (and each one has different behavior).

https://dollarshaveclub.github.io/router-scroll-demo/

https://dollarshaveclub.github.io/ember-router-scroll/

I've also noticed the dummy app in this addon does not work correctly.

Wondering which of these apps should be the source of truth?

Improve readme structure / order to help new users

I was nearly pulling my hear out trying to figure out why ember-router-scroll wasn't working in my app, until I happened to scroll down past the examples and find that the "Usage" section actually contains crucial instructions on setting it up.

This would hopefully be less of an issue if the installation and usage sections weren't separated by more than a screen worth of examples etc.

Broken builds with 0.5 release

Hi there, I upgraded an addon using this as a dependency from router-scroll v0.4 to v0.5 and I'm seeing broken builds across a number of versions of Node and Ember.

You can see them here: https://travis-ci.org/jeffjewiss/ember-cli-postcss/builds/337610492

I downgraded to v0.4 and the builds are passing again: https://travis-ci.org/jeffjewiss/ember-cli-postcss/builds/337628529

I'll need to look into this further, but it may be related to #85

Change timing of window.scrollTo

Currently window.scrollTo is fired in didTransition which itself is wrapped in an Ember.runloop call. And the transition is fired from within the runloop. The result is that there is a jitter when scrolling to a different position. The page renders and there is a noticeable jump. It is fast but noticeable.

I believe the scroll positioning should be done elsewhere. It would be great if we could hook into an afterRender callback. Not sure if one is fired for the entire page.

New Release?

There are a lot of new features/bugfixes (like FastBoot compatibility) in this addon since the last release in Nov 2017. Can we cut a new release so that people trying this addon for the first time don't get a broken experience because they're not on master?

Mimic back/forward behavior on normal transitions

Does the addon support a way to return the user to the saved scroll position for a previous page without having to be invoked by a "Back/Forward" browser event, i.e. via clicking a {{link-to}} that sends the user back to that previous page?

The scenario is a common master list/detail view case, where the user scrolls through a list of items, selects one to navigate to a detail view, the clicks a "Back to list" link, which I would like to return the user to their previous scroll position in the list, like what happens if they click the browser Back button. Currently, this returns them back to the top of the list.

If the addon does not support this, is there viable way to extend it to do so, or a recommended path to achieving this outside outside of ember-router-scroll altogether?

Scroll position for historical pages

Visit page A, scroll down
From page A, click on Link to page B, scroll down
Back button to page A, previous scroll position is preserved.

Visit page A, scroll down
From page A, click on Link to page B, scroll down
From page B, click on Link to page A
Back button twice to page A, previous scroll position is not preserved.

Any reason scrollElement needs an ID?

Could we change scrollElement to accept any CSS selector, like a class?

Getting currentURL from a routes router property triggers deprecation

The line that illicits the deprecation:

https://github.com/dollarshaveclub/ember-router-scroll/blob/dc98e4773a2a933de4163c8f9cf6dba7938781c8/addon/index.js#L35

The immediate solution is to use _router which is considered an intimate API per the deprecation notice. (which can be found here)

https://deprecations-app-prod.herokuapp.com/deprecations/v3.x/#toc_ember-routing-route-router

Consider use instance-initializer instead of mixin

It would be easier to maintain if the logic currently in the mixin lives in an instance initializer. Just like the current mixin, having the logic in the instance initializer maintains the benefit of centralizing the scroll position management logic in one place, but without requiring individual routers to include the mixin.

This is especially beneficial in a large ember project involving many developers and many route files. It would be much easier to adopt this addon without having to modify all the route files and informing all developers on the team that they need to wire in this mixin. It will be more automatic. This is assuming the use case that most routes in the app would like the behavior provided by the mixin.

Add tests

Back button broken

As of 0.7.0, if I visit the index route of my app, scroll, then visit another route, and try to hit the back button to go back, the url changes, but my transition to the other route does not happen.

Feature Discussion: targetElement to scroll to

For non scrollElement configs, It would be great to scroll to a y coordinate that is not window (y: 0). Perhaps a config called targetElement and window.scrollTo(x,y) would take the x & y coordinates from the targetElement.offsetTop.

Also, the reason why scrollElement does not work is that the target element may not be scrollable, thus it's content not generating a scrollbar (and it's scrollTop == 0 all the time).

I can submit a PR if this sounds good to everybody!

Git tags are missing

The latest git tag in this repository seems to be https://github.com/dollarshaveclub/ember-router-scroll/releases/tag/0.1.0, but according to the recent commits the latest release is v0.4.0.

[Update] add more tests

Add more tests

For this issue more testing will be implemented.

Reasons 🤔

• easier code review
• tests need to be added for future features like:
  • post-merge publishing and changelog updates,
  • automerge

---

Steps 📊

The checklist key items will be in separate PRs for readability.
Versioning for these PRs will try to best support the community.

---

Key Items ✅

• Add unit tests specific to location/router-scroll.js
• Add unit tests specific to services/router-scroll.js
• Add acceptance tests
• Spike: Other test opportunities?

Use Circle to publish 🚀

https://docs.travis-ci.com/user/deployment/npm
https://github.com/npm/node-semver

Getting error "Cannot read property 'afterContentPaint' of null" when running tests

Occasionally on our build server, I get a failing test with the following error: Cannot read property 'afterContentPaint' of null

There are only 2 hits on Google for afterContentPaint, and one of them is this issue: ember-app-scheduler/ember-app-scheduler#9

This addon seems to be the only thing using that library. Have you ran into this error while using ember-router-scroll?

Update documentation to say that 'service:scheduler' is also required in route/controller tests

When running unit tests, I get this error:

Assertion Failed: Attempting to inject an unknown injection: 'service:scheduler'

The documentation says that service:router-scroll must be added to the needs: [] block for router/controller tests, but doesn't mention anything about service:scheduler.

Is service:scheduler required for all configurations as well? If so, this needs to be added to the documentation.

[Feature] Implement CircleCI

Implement CircleCI

For this issue CircleCi will/can be implemented.

Reasons 🤔

• support from the DSC team
• faster CI
• automate post-merge features

---

Steps 📊

The checklist items will be in separate PRs for readability.
Versioning for these PRs will try to best support the community.

---

Key Items ✅

• adds/update eslinting — re-adds dsc rules again.
• Init CircleCi
• Add ES-Check
• Remove Travis
• Init CodeCov
• Add tests—not a checklist item, this is another issue.
• Add post-merge featuring—not a checklist item, this is another issue (This issue will be added closer to implementation time).
  • Publishing
  • Auto PR/merge changelog

Update README to explain how it really works now as of 0.0.5 and newer

Since the inner workings were refactored, the how it works section is no longer accurate

Create GH Pages as an example of the add-on working

Error when navigating to url with nothing after anchor #

We're seeing a Cannot read property 'offsetLeft' of null error when navigating to a url with a trailing hash (e.g. http://example.com/home#).

Looks to me like the culprit is this line here https://github.com/dollarshaveclub/ember-router-scroll/blob/master/addon/index.js#L38, as we're hitting that if condition even though there's nothing to be assigned to hashElement.

router-scroll location type breaks non-root routes in development

Create new ember-cli app:

1. ember new router-scroll-test
2. cd router-scroll-test
3. Remove ember-welcome-page addon
4. ember generate route nested
5. Add content to app/templates/nested.hbs
6. ember serve
7. Visit localhost:4200/nested (content loads fine)

Introduce ember-router-scroll:

1. ember install ember-router-scroll
2. Follow README setup (import mixin into router.js; set locationType: 'router-scroll')
3. Restart development server with ember serve
4. Visit localhost:4200/nested

It seems that something in the 'router-scroll' locationType is breaking the development express server's ability to load the app directly into a non-root route. This is especially frustrating when livereload is constantly refreshing the page during development (triggering this error each time).

Move demo into `tests/dummy`

That way we do not have to maintain it separately. From this repo.

scroll to top not working with module unification?

I have a bunch of things going on in this project, but this is my attempt to get ember-router-scroll working: NullVoxPopuli/emberclear@589303b

silly things I'm doing:

• module unification
• es classes (should only be relevant for routes?)
• fastboot & prember

Anything obvious stand out?

Rebasing got rid of important code

replace add-on/index.js with the following

import Ember from 'ember';

const {
  get,
  inject,
  run: { next },
} = Ember;

export default Ember.Mixin.create({
  service: inject.service('router-scroll'),

  willTransition(...args) {
    this._super(...args);
    get(this, 'service').update();
  },

  didTransition(transitions, ...args) {
    this._super(transitions, ...args);
    next(() => {
      let scrollPosition = get(this, 'service.position');

      let preserveScrollPosition = transitions[transitions.length - 1]
        .handler.controller.get('preserveScrollPosition');

      if (!preserveScrollPosition) {
        window.scrollTo(scrollPosition.x, scrollPosition.y);
      }
    });
  }
});

Missing tags

Looks like some tags are missing from Github when comparing to published versions on npm.

Uncaught Error: Could not find module `ember-getowner-polyfill`...

I get the following error after installing and making it to Step 5: Profit:

Could not find module ember-getowner-polyfill imported from ember-router-scroll/index

Once I manually installed ember-getowner-polyfill it worked fine.

Update [email protected]

It would be nice to update ember-cli and ember-source to 3.0 as well as the new testing syntax. However, that could come after #98 #96 #97 if possible. Then follow with #99

@rwwagner90 @briangonzalez Lmk what you think!

Write demo application and link to it in the readme

scroll position reset on slow loading fastboot served pages

I know why this is happening, I'm not sure what the correct solution is yet.

If a page is served with Fastboot the immediate render is available. Then if Ember is slow to instantiate for any reason if you've scrolled down the page when the App starts the scroll position is reset. This is not the correct experience.

I suspect the correct thing to do is if the state is new assume the current scroll position rather than 0,0

Consider moving mixin location

The README states you should use:

import RouterScrollMixin from 'ember-router-scroll';

However the mixin is actually located elsewhere, so you have to do:

import RouterScrollMixin from 'ember-router-scroll/mixins/router-scroll';

which is OK but the line in the README seems better. To achieve this you can move ember-router-scroll/addon/mixins/router-scroll.js to ember-router-scroll/addon/index.js

create router-scroll-history and router-scroll-hash location types

Ember 3.0 - browser refresh causes back button to stop working

After upgrading to ember v3.0, I've noticed that in a particular scenario the back button fails to produce a template render on the previous route. Steps to reproduce:

1. Open new tab in Chrome
2. navigate to initial application url (i.e. localhost:4000/search)
3. do a browser page refresh
4. transition to a new route (e.g. localhost:4000/results)
5. hit the browser back button

At this point, I see the previous route name in the URL bar (localhost:4000/search), however there is no actual application transition, no rendering of that route, nothing but a history state change in the URL bar.

I tried creating a new Ember app from scratch, tested this type of route transition without router-scroll, then installed router-scroll and tested again, and can confirm that the bug is only present with the addon install.

Force urls to have a trailing slash

I have been working on converting my app to use prember, and I had a need to force trailing slashes. I am extending the router-scroll location type to achieve this. Would there be interest in adding support here directly with a flag to enable?

import EmberRouterScroll from 'ember-router-scroll/locations/router-scroll';

export default EmberRouterScroll.extend({
  formatURL() {
    let url = this._super(...arguments);

    if (url.includes('#')) {
      return url.replace(/([^/])#(.*)/, '$1/#$2');
    } else {
      return url.replace(/\/?$/, '/');
    }
  }
});

Add in HashLocation version

ember install ember-router-scroll not installing 0.3.0

I feel like a total nube here, but this has never happened to me before. I just moments ago ran ember install for this add-on. I am in need of the new feature "scrollElement". The code that npm is pulling to my machine does not have this feature. The addon/index.js file that is on my machine does not look like what is in master for 0.3.0. It seems that I am getting the previous version.

Ember 3.1.x deprecation warning

DEPRECATION: Route#router is an intimate API that has been renamed to Route#_router. However you might want to consider using the router service [deprecation id: ember-routing.route-router]

The deprecation warning relates to index.js#L35. I believe it relates to Route.router being renamed to Route._router in Ember 3.x and recommended access of router properties now being through the router service.

Uncaught Error: Could not find module

Uncaught Error: Could not find module `ember-scroll-operator/mixins/reset-scroll` 
imported from `website/open-source/route`

Any ideas why it can't be imported?

Attempting to inject an unknown injection: 'service:scheduler'

When running tests on a project using [email protected] and [email protected], all of my tests blow up with the following error:

Assertion Failed: Attempting to inject an unknown injection: 'service:scheduler'

Update README.md to explain how it works

Needs info on HistoryLocation API

Add support for navigating to a "#" (hash) url.

Hi!

If I have a link like:
example.com/hash#hashId

It would be nice if the app would properly navigate to it. I can think of a really quick and dirty fix for it in this function:

https://github.com/dollarshaveclub/ember-router-scroll/blob/2d577cd4a695310e02db1217d9f8eff5f9bdf1d1/addon/index.js#L30

  updateScrollPosition(transitions) {
    const lastTransition = transitions[transitions.length - 1];
    const url = lastTransition.handler.get('router.currentURL');
    
    let scrollPosition;
    if(url.includes("#")) {
      const hashElement = document.getElementById(url.split("#").pop());
      scrollPosition = { x: hashElement.offsetLeft, y: hashElement.offsetTop }
    } else {
      scrollPosition = get(this, 'service.position');
    }
    ... Rest of the code here ...
  }

Let me know if this is something you would like or alternatively, that I'm completely wrong and you do in fact support # links.

Can we use the 'history' type for url type?

It looks like that when setting the locationType to router-scroll it will behave like hash. The project I am working on we use the history type. Do I configure something wrong or is this the intended behavior of the addon?

github url in package.json

also add npm contributors

Add support for hashlocation

It will be cool if ember-router-scroll has support for hash location. So i can use locationType: 'router-scroll-hash'.

Is this addon still needed in Ember 2.13?

I got thrown off by the note in the readme for this project and my experience with using this addon in Ember 2.13.

RFC #186 has made it into Ember 2.13 but the scroll behaviour I was looking is only there if I use this add-on. Am I missing something to enable the scrolling behaviour in Ember core?

get this up on npm via DSC's account

if a nested route is within a fixed height container, the reset scroll doesn't work.

The scenario is if you have a sticky header and sticky footer.

My main div that all my routes are rendered in has these styles:

// sticky-footer styles
.inner-wrap {
  height: 100%;
  height: 100vh; // this and the overflow causes the app scroll to be in a nested scrollbar.
  overflow-x: hidden;
  background: white;
  min-height: 100%;
  display: flex;
  flex-direction: column;

  section.main-section, .shell, .inner-shell {
    flex: 1;
    // my app pretty much lives here
  }

  .sticky-footer {
    margin-top: 40px;
  }
}

So, I guess is there a way to specify what dom element has its scroll reset? maybe an array of selectors to specify?

Will not work with FastBoot

pull request on the way!

safari resets scroll position

Hi,

the plugin is working fine on firefox and chrome, unfortunately it is not working on safari (10.1.1) also on safari mobile (sorry unknown version).
Using ember 2.14, and ember-router-scroll 0.1.1.

Any ideas?
thanks!