[Bugfix]Fix autoscroll to methods issue #321

akashdsouza · 2017-08-10T08:52:28Z

If this looks ok, I can add the same to /methods under classes and modules(perhaps in a mixin?).

sivakumar-kailasam · 2017-08-16T17:11:05Z

Fixes #318

MartinMalinda · 2017-08-16T17:15:03Z

app/routes/project-version/namespaces/namespace/methods.js

+  actions: {
+    didTransition() {
+      this._super();
+      if ((typeof FastBoot === 'undefined') && isEmpty(this.get('controller.anchor'))) {


I think I've mentioned it before somewhere. isEmpty(this.get('controller.anchor')) this will probably result in scroll jump unless there is anchor specifically set up. Which is probably not a good behavior.

@sivakumar-kailasam @locks
Do we need to have backward compatibility on this? Or could we come up with a more specific anchor for scrolling to methods?

Unfortunately we do. There's just so many old links out there outside our website that refer to the old doc urls. We can probably do an analysis of google analytics metrics to see the trends few months down the line and make changes but until then we have to keep at this.

@akashdsouza
Oh okay. Then I think we need to specifically check for null or "" (or both?) so it only detects just ?anchor=

Hopefully, that will work and there won't be a need to look into location.

Alright. I will update the check.
Should I use a data-attribute instead of id for scrolling to the tab?

I think ID is fine.

akashdsouza · 2017-08-16T20:10:52Z

This might have to wait until #322 is merged

MartinMalinda · 2017-08-17T19:53:22Z

app/mixins/scroll-page.js

+          $(config.APP.scrollContainerSelector).scrollTop(offset - navMenuHeight - 10);
+        }
+      } else {
+        this.get('scrollPositionReset').doReset();


Why is the reset needed if there's no anchor?

To retain the behavior available here:

ember-api-docs/app/mixins/scroll-tracker.js

Line 14 in 7943dba

this.get('scrollPositionReset').doReset();

Earlier, as there was no didTransition in /methods, didTransition above would have been called.

MartinMalinda · 2017-08-17T19:55:10Z

app/routes/project-version/classes/class/methods.js

@@ -1,5 +1,6 @@
 import Route from '@ember/routing/route';


I think the correct place to put the Mixin would be project-version/classes/class.js and project-version/modules/module.js and project-version/namespaces/namespace.js. That should cover all cases.

There is already a scroll-tracker mixin in these routes to reset scrolling. How would this need to be handled? Remove the scroll reset altogether as it is present in scroll-page mixin?

I think we could actually add this logic to the Scroll Tracker mixin itself. It is already at the right places - maybe it will just need to be added to namespaces/namespace.js.

MartinMalinda · 2017-08-17T20:01:00Z

Works well, but the Mixin should be put in different places as I've suggested above, otherwise, the scroll jump may not happen in various places.

I think we should also consider updating these query params

ember-api-docs/app/templates/project-version/classes/class.hbs

Line 50 in 7020a59

(query-params anchor="")

There is a query-param reset like this (query-params anchor=""). I think the reason for that was so there would not be a random jump to some method - but now it causes a scroll jump to the tab element. (query-params anchor=undefined) should be a better alternative now.

akashdsouza · 2017-08-18T03:46:04Z

app/mixins/scroll-tracker.js

-      this.get('scrollPositionReset').doReset();
+      this._super();
+      let isAnchorEmpty = window && window.location && window.location.search === '?anchor='
+      if ((typeof FastBoot === 'undefined') && isAnchorEmpty ) {


Does this seem hacky?

Getting anchor from the controller cannot be done from these places?
If we are not in FastBoot, it is safe to reach into window.location.search right away. (typeof FastBoot === 'undefined') && window.location.search === '?anchor=') should be fine.
This will also break if we have more than 1 queryParam, but this is probably fine. So far there no plans for adding more.

Yes. anchor is only available in child route's(methods) controller.
I will update the check

akashdsouza · 2017-08-18T03:49:18Z

app/mixins/scroll-tracker.js

+        let offset = (elem && elem.offset && elem.offset()) ? elem.offset().top : null;
+        if (offset) {
+          const navMenuHeight = $('header').outerHeight();
+          $(config.APP.scrollContainerSelector).scrollTop(offset - navMenuHeight - 10);


Since header is no longer a fixed element on the page, do we still need to take navMenuHeight into consideration? Having a little space above the method doesn't look all bad

Having a little space above the method doesn't look all bad

There will be less space if we remove it :). But let's do it, I think the anchor is gonna be more clear.

akashdsouza · 2017-08-18T07:04:07Z

Updated

MartinMalinda · 2017-08-18T07:38:17Z

app/mixins/scroll-tracker.js

+      this._super();
+      if ((typeof FastBoot === 'undefined') && window.location.search === '?anchor=' ) {
+        let elem = $('#methods');
+        let offset = (elem && elem.offset && elem.offset()) ? elem.offset().top : null;


This is also overly defensive. If it's JQuery object, it is safe to assume that elem.offset will be a function even if the element is not found.

let offset = elem.offset() ? olem.offset().top : 0 should be fine.

Updated. Sorry about the delay

MartinMalinda · 2017-08-18T07:40:34Z

Works well 💯 The offset check could be simplified but it's also fine as is.

toddjordan · 2017-08-31T18:31:20Z

looks good, thanks @akashdsouza !!!

* [Bugfix]Fix autoscroll to methods issue * Do not include header height while calculating scroll offset

akashdsouza mentioned this pull request Aug 10, 2017

[BUGFIX]Fix broken method link #319

Merged

locks temporarily deployed to ember-api-docs-staging-pr-321 August 11, 2017 02:49 Inactive

sivakumar-kailasam requested a review from MartinMalinda August 16, 2017 17:09

MartinMalinda suggested changes Aug 16, 2017

View reviewed changes

sivakumar-kailasam mentioned this pull request Aug 16, 2017

Autoscroll to methods section is broken #318

Closed

locks temporarily deployed to ember-api-docs-staging-pr-321 August 16, 2017 18:24 Inactive

akashdsouza force-pushed the bugfix_autoscroll_methods branch from c7dd461 to 3daf6e6 Compare August 16, 2017 20:03

locks temporarily deployed to ember-api-docs-staging-pr-321 August 16, 2017 20:03 Inactive

akashdsouza force-pushed the bugfix_autoscroll_methods branch from 3daf6e6 to d418399 Compare August 17, 2017 19:52

locks temporarily deployed to ember-api-docs-staging-pr-321 August 17, 2017 19:52 Inactive

MartinMalinda reviewed Aug 17, 2017

View reviewed changes

akashdsouza force-pushed the bugfix_autoscroll_methods branch from d418399 to e4f1924 Compare August 18, 2017 03:08

locks temporarily deployed to ember-api-docs-staging-pr-321 August 18, 2017 03:09 Inactive

[Bugfix]Fix autoscroll to methods issue

24391cf

akashdsouza force-pushed the bugfix_autoscroll_methods branch from e4f1924 to 24391cf Compare August 18, 2017 03:45

locks temporarily deployed to ember-api-docs-staging-pr-321 August 18, 2017 03:45 Inactive

akashdsouza commented Aug 18, 2017

View reviewed changes

locks temporarily deployed to ember-api-docs-staging-pr-321 August 18, 2017 07:01 Inactive

MartinMalinda approved these changes Aug 18, 2017

View reviewed changes

Do not include header height while calculating scroll offset

43d462f

akashdsouza force-pushed the bugfix_autoscroll_methods branch from d7815d6 to 43d462f Compare August 20, 2017 18:08

locks temporarily deployed to ember-api-docs-staging-pr-321 August 20, 2017 18:09 Inactive

locks temporarily deployed to ember-api-docs-staging-pr-321 August 31, 2017 16:40 Inactive

toddjordan merged commit 42c02b5 into ember-learn:master Aug 31, 2017

lifeart pushed a commit to lifeart/ember-api-docs that referenced this pull request Apr 18, 2018

[Bugfix]Fix autoscroll to methods issue (ember-learn#321)

31a7198

* [Bugfix]Fix autoscroll to methods issue * Do not include header height while calculating scroll offset

Uh oh!

[Bugfix]Fix autoscroll to methods issue #321

[Bugfix]Fix autoscroll to methods issue #321

Uh oh!

Conversation

akashdsouza commented Aug 10, 2017

Uh oh!

sivakumar-kailasam commented Aug 16, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akashdsouza commented Aug 16, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MartinMalinda commented Aug 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akashdsouza Aug 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akashdsouza commented Aug 18, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MartinMalinda commented Aug 18, 2017

Uh oh!

toddjordan commented Aug 31, 2017

Uh oh!

Uh oh!

MartinMalinda commented Aug 17, 2017 •

edited

Loading

akashdsouza Aug 18, 2017 •

edited

Loading