[Major]: refactor: use`RouterService` instead of Ember `Router` #270

snewcomer · 2020-12-24T18:30:06Z

close #256 close #255

This will be released as a major bump -> 4.0

remove locations/router-scroll.js. This has been deprecated and you should rely on Ember's own implementation instead of extending from us. HistoryLocation Subclass Question #193
Remove EmberRouterScroll that you used to import and extend in your application. We handle that for you now and eagerly initialize the service for you. We did this because we can rely on RouterService events instead of this.

This fixes the following error: ``` Build Error (broccoli-persistent-filter:Babel > [Babel: @ember/test-helpers]) in @ember/test-helpers/dom/-is-focusable.js Cannot find module '@babel/compat-data/corejs3-shipped-proposals' Require stack: - /Users/jan/open-source/ember-router-scroll/node_modules/@babel/preset-env/lib/polyfills/corejs3/usage-plugin.js - /Users/jan/open-source/ember-router-scroll/node_modules/@babel/preset-env/lib/index.js - /Users/jan/open-source/ember-router-scroll/node_modules/@babel/core/lib/config/files/plugins.js - /Users/jan/open-source/ember-router-scroll/node_modules/@babel/core/lib/config/files/index.js - /Users/jan/open-source/ember-router-scroll/node_modules/@babel/core/lib/index.js - /Users/jan/open-source/ember-router-scroll/node_modules/broccoli-babel-transpiler/lib/worker.js ```

snewcomer · 2020-12-25T02:34:48Z

@buschtoens 👋 Let me know what you think!!

snewcomer · 2020-12-29T14:05:32Z

@simonihmig as well if you have a moment!

#193

addon/services/router-scroll.js

Co-authored-by: John Leja <[email protected]>

jleja

Looks good. Just had two things to point out.

jleja · 2021-01-05T15:37:24Z

addon/services/router-scroll.js

+      preserveScrollPosition = get(this, 'preserveScrollPosition')
+    }
+
+    if (!preserveScrollPosition) {


Can combine the two if (!preserveScrollPosition) blocks

jleja · 2021-01-05T15:51:21Z

addon/services/router-scroll.js

+      // out of the option, this happens on the tightest schedule
+      scheduleOnce('render', this, CALLBACK, transition);
+    } else if (scrollWhenAfterRender && !scrollWhenIdle) {
+      // out of the option, this happens on the tightest schedule
+      scheduleOnce('afterRender', this, CALLBACK, transition);


Are the duplicated comments here correct, or is scheduling after render the option that happens on the tightest schedule?

cbroeren

I wasn't able to verify this addon with a running app yet, but left some comments. Looks 👍 overall

cbroeren · 2021-01-05T14:25:21Z

CHANGELOG.md

@@ -2,6 +2,9 @@

 - Updating to th v3.0 series was due to removing `scrollWhenPainted` as a config option.  Also, we fixed some hidden bugs with scheduling when to scroll to your last y position.

+## 4.0.0
+* [#270](https://github.com/DockYard/ember-router-scroll/pull/270) [Major]: use`RouterService` instead of Ember `Router`


Would it be helpful to also mention the changes users should make on updating to this version? Or do you expect them to find that in the PR?

Great idea! I updated the README.

cbroeren · 2021-01-05T16:59:21Z

addon/services/router-scroll.js

+
+    // If `preserveScrollPosition` was not set on the controller, attempt fallback to `preserveScrollPosition` which was set on the router service.
+    if (!preserveScrollPosition) {
+      preserveScrollPosition = get(this, 'preserveScrollPosition')


Mind adding this logic to the initial assignment? Double assignment and the two same if blocks are a bit confusing.

That is a good idea!

cbroeren · 2021-01-05T17:02:23Z

addon/services/router-scroll.js

+    if (url && url.indexOf('#') > -1 && hashElement) {
+      scrollPosition = { x: hashElement.offsetLeft, y: hashElement.offsetTop };
+    } else {
+      scrollPosition = get(this, 'position');


What if you use this as the initial value (it gets overwritten in the if block if needed)

Another good find. Thank you Chantal!

addon/services/router-scroll.js

cbroeren · 2021-01-05T17:09:17Z

addon/services/router-scroll.js

+
+    if (!preserveScrollPosition) {
+      const scrollElement = get(this, 'scrollElement');
+      const targetElement = get(this, 'targetElement');


Do you need the get or could you use const { scrollElement, targetElement } = this; ?

cbroeren · 2021-01-05T17:10:30Z

addon/services/router-scroll.js

+      // out of the option, this happens on the tightest schedule
+      scheduleOnce('render', this, CALLBACK, transition);
+    } else if (scrollWhenAfterRender && !scrollWhenIdle) {
+      // out of the option, this happens on the tightest schedule


Should this be the same comment as above?

Co-authored-by: Chantal Broeren <[email protected]>

buschtoens and others added 3 commits July 29, 2020 17:47

refactor: first shot at using RouterService

9d413d0

Merge branch 'master' into pr/256

0b6c4ac

snewcomer self-assigned this Dec 24, 2020

snewcomer added 4 commits December 24, 2020 12:37

refactor to master

259d624

fix lockfile

22a6536

Merge branch 'master' into pr/256

405dab4

some improvements to tests. more to go

3dfcddf

snewcomer force-pushed the pr/256 branch from 5aae75d to 3dfcddf Compare December 24, 2020 20:36

snewcomer added 3 commits December 24, 2020 16:51

fix some tests

4d2594c

Fix tests

a04c138

Merge branch 'master' into pr/256

959523a

snewcomer added the enhancement label Dec 25, 2020

snewcomer changed the title ~~Pr/256~~ refactor: useRouterService instead of Ember Router Dec 25, 2020

snewcomer added 2 commits December 24, 2020 18:48

fix bug in recursive call

426cea9

Update docs

1869d10

snewcomer added 2 commits January 4, 2021 13:42

rm deprecation outdated

b430c71

Do not need locations/router-scroll

344da15

#193

snewcomer changed the title ~~refactor: useRouterService instead of Ember Router~~ [Major]: refactor: useRouterService instead of Ember Router Jan 5, 2021

CHANGELOG 4.0.0

a2ccbdb

snewcomer requested review from cbroeren, jleja and Ravenstine January 5, 2021 13:52

jleja reviewed Jan 5, 2021

View reviewed changes

addon/services/router-scroll.js Outdated Show resolved Hide resolved

Update addon/services/router-scroll.js

5523cb6

Co-authored-by: John Leja <[email protected]>

jleja approved these changes Jan 5, 2021

View reviewed changes

cbroeren reviewed Jan 5, 2021

View reviewed changes

fix comment

a6671dc

snewcomer and others added 4 commits January 5, 2021 20:41

Address some feedback

047d3cc

Update addon/services/router-scroll.js

1592ea5

Co-authored-by: Chantal Broeren <[email protected]>

moar updates

2d31198

address moar feedback

6ffa46a

snewcomer merged commit 27db57f into master Jan 6, 2021

snewcomer deleted the pr/256 branch January 6, 2021 02:55

mattmcmanus mentioned this pull request Jan 25, 2021

Remove app file for router-scroll, setup embroider tests #274

Merged

SergeAstapov mentioned this pull request Mar 25, 2021

Run service teardown code in willDestroy() #280

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Major]: refactor: use`RouterService` instead of Ember `Router` #270

[Major]: refactor: use`RouterService` instead of Ember `Router` #270

snewcomer commented Dec 24, 2020 •

edited

Loading

snewcomer commented Dec 25, 2020

snewcomer commented Dec 29, 2020

jleja left a comment

jleja Jan 5, 2021

jleja Jan 5, 2021

cbroeren left a comment

cbroeren Jan 5, 2021

snewcomer Jan 6, 2021

cbroeren Jan 5, 2021

snewcomer Jan 6, 2021

cbroeren Jan 5, 2021

snewcomer Jan 6, 2021

cbroeren Jan 5, 2021

cbroeren Jan 5, 2021

[Major]: refactor: useRouterService instead of Ember Router #270

[Major]: refactor: useRouterService instead of Ember Router #270

Conversation

snewcomer commented Dec 24, 2020 • edited Loading

snewcomer commented Dec 25, 2020

snewcomer commented Dec 29, 2020

jleja left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cbroeren left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

[Major]: refactor: use`RouterService` instead of Ember `Router` #270

[Major]: refactor: use`RouterService` instead of Ember `Router` #270

snewcomer commented Dec 24, 2020 •

edited

Loading