fixed scrollPercent calculation for GridRenderContainer and InfiniteScroll Feature #5395

vance · 2016-05-10T22:29:36Z

fixed scrollPercent calculation for GridRenderContainer and InfiniteScroll Feature

fixes for
#5211
and
#4508

…the Feature Infinite Scroll. fixes for angular-ui#5211 and angular-ui#4508

swalters · 2016-06-01T13:16:42Z

src/features/infinite-scroll/js/infinite-scroll.js

@@ -22,6 +22,16 @@
   */
  module.service('uiGridInfiniteScrollService', ['gridUtil', '$compile', '$timeout', 'uiGridConstants', 'ScrollEvent', '$q', function (gridUtil, $compile, $timeout, uiGridConstants, ScrollEvent, $q) {

+
+    // not sure where to put this util function @vance


You could move this to the GridRenderContainer class

@vance Are you planning on taking @swalters's suggestion?

ehh. splitting time between a fortune 500 a startup and marketing my own side-app at the moment. I don't know the next time I will have the headspace to compile this again, christmas time?

jrthib · 2016-07-17T01:38:24Z

Any news on this fix? One of my applications seems to be affected by it.

hexaclock · 2016-07-25T01:19:23Z

Any word on this getting merged into master?

vance · 2016-08-09T17:32:34Z

I'll take this as a compliment. Seems like the PR is just dinged for being complex? I'm not sure how to make a less-complex fix for one of the most complex parts of this library. It took 2 days to get my brain into the space where I could understand the problem, let alone fix it. There's no way I could refactor it now, if that's even possible to do so without increasing the complexity. So, does the project want to fix a fatal bug that essentially breaks all expandable row templates, or reject a PR because it is complex? I'm not sure, I don't maintain libraries of this size.

vance · 2016-08-09T18:49:30Z

For anyone in a pinch, i made a gist of the compiled code. I've been using this fix in production for weeks with no issues.

mportuga · 2016-09-20T20:56:37Z

@vance Can you please fix unit tests?

vance · 2016-09-21T00:05:37Z

Does CI not run the tests? Er, what tests are breaking?

I tried running grunt dev, grunt dev --browsers=Chrome and I see karma launches a bunch of chromes, but I don't see anything happening.

cs45977 · 2016-10-03T18:38:57Z

Any news on this PR?

vance · 2016-10-03T18:53:35Z

I see the removed needs fixed test... I don't recall any tests breaking. I did not have time to add new tests as I already went over my budget fixing this. But I don't think that should prevent a merge. Anyone else besides me manually verify correctness of the expanded row calculations? I'm running this in prod with no issues.

cs45977 · 2016-10-05T19:51:55Z

@vance In your Gist:
It looks the file is truncated. Anyway you can provide me your full file?

vance · 2016-10-05T20:50:49Z

Oh, sorry. I guess Gist has a limit of about 1MB. I put it on my server, here:

http://www.foreverscape.com/angular-grid-ui-exp-fix.js
It's github pages, anyway, haha.

cs45977 · 2016-10-15T01:21:35Z

Bump @mportuga
It looks like this should be merged ... complex solution for a complex problem
no?

mportuga · 2016-10-18T13:49:21Z

src/js/core/factories/GridRenderContainer.js

+          // var totalNumRowsToRender = Math.min(rowCache.length,rowIndex + minRows + self.grid.options.excessRows);
+
+          //TODO: @vance calculate and appropriate end... doesn't break things, just show more rows than necessary here.
+          var rangeEnd = rowCache.length;


Showing more rows than necessary is considered a defect for some companies. Can you describe what you mean by that? Will the scrolling end, or will it just keep going since this is the infinite scroll library?

mportuga · 2016-10-18T13:50:08Z

src/js/core/factories/GridRenderContainer.js


-    self.updateViewableColumnRange(newRange);
+        // Define a max row index that we can't scroll past


This code seems to be describing a max column index, not a max row index.

not added by me, maybe moved.

mportuga · 2016-10-18T13:51:07Z

src/js/core/factories/GridRenderContainer.js

+          if (!(typeof(scrollTop) === 'undefined' || scrollTop === null)) {
+            // Have we hit the threshold going down?
+            if ( !self.grid.suppressParentScrollDown && self.prevScrollTop < scrollTop && rowIndex < self.prevRowScrollIndex + self.grid.options.scrollThreshold && rowIndex < maxRowIndex) {
+              console.log('returning first bail');


You should comment this out, we don't want the grid logging this in production.

mportuga · 2016-10-18T13:51:13Z

src/js/core/factories/GridRenderContainer.js

+            }
+            //Have we hit the threshold going up?
+            if ( !self.grid.suppressParentScrollUp && self.prevScrollTop > scrollTop && rowIndex > self.prevRowScrollIndex - self.grid.options.scrollThreshold && rowIndex < maxRowIndex) {
+              console.log('returning second bail');


You should comment this out, we don't want the grid logging this in production.

ionutb · 2016-11-02T09:40:09Z

vance, I can't thank you enough, that fix was critical to our project. I hope the patch will be merged soon.

mportuga · 2016-11-07T21:31:14Z

@dlgski This appears to be good and it will fix a big issue for many people. Since we are planning a major update, maybe now is the time to take this?

mportuga · 2016-11-16T18:35:47Z

@vance Can you resolve merge conflict?

vance · 2016-11-21T17:55:12Z

Nope. It took me two full days to solve this. I was told to fix tests that
weren't broken. I was told to fix cyclomatic complexity by a
robot....several months later. I made this fix MONTHS ago, I don't even
remember now it works now. Maybe in January I will have some time.

On Wed, Nov 16, 2016 at 10:35 AM Marcelo Sauerbrunn Portugal <
notifications@github.com> wrote:

@vance https://github.com/vance Can you resolve merge conflict?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#5395 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAI4PxBj1QQXGY_IK6OYpW9mrwy1jK7uks5q-00KgaJpZM4IbkpZ
.

mportuga · 2016-12-05T15:41:28Z

Closing since the developer needs more time to fix it up.

BPoorey · 2017-03-07T09:52:47Z

Any updates on this?

nonongodup · 2018-08-02T18:01:23Z

any updates on merging this ? my application is affected by the same issue. it would be good if you can merge. this would solve problem to many.

mportuga · 2018-08-05T00:27:58Z

This was closed due to inactivity, i will reopen and see if I can finish the work

mportuga · 2018-08-06T15:30:11Z

After reviewing this code, I realized that it does not actually address the problem in an acceptable way. All this code really does in the end is get rid of the virtualization, which would be horrible for performance.

nonongodup · 2018-08-06T15:51:50Z

It does affect the efficiency. with this code the page loading become little slow. Is there any other solution for this issue?

mportuga · 2018-08-06T17:23:11Z

None that I am aware off, and unfortunately, I don't know when I will have enough time to look at this more throughly. I will certainly accept other pull requests if someone else finds it, but for the next couple of weeks, I will be traveling out of the country, followed shortly by moving to a new place, so I won't have much time to look into ui-grid.

nonongodup · 2018-11-20T15:48:07Z

@mportuga any update on this bugs ?

Southpaw17 · 2018-12-17T15:24:55Z

src/js/core/factories/GridRenderContainer.js

+
+        } else {
+          var maxLen = self.visibleRowCache.length;
+          newRange = rowCache.length;//[0, Math.max(maxLen, minRows + self.grid.options.excessRows)];


@mportuga This is the line you were looking for

This means that this PR is invalid. It simply removes the virtual scrolling

fixed the scrollPercent calculation for both GridRenderContainer and …

dc183a1

…the Feature Infinite Scroll. fixes for angular-ui#5211 and angular-ui#4508

swalters reviewed Jun 1, 2016
View reviewed changes

AgDude added needs: fix tests and removed needs: fix tests labels Jul 30, 2016

This was referenced Aug 25, 2016

Row virtualization is working incorrectly in expandable grid #5475

Open

Scrollbar BUG: Scrollbar doesn't work in the after expand the subGrid #5587

Open

mportuga added the waiting-response label Sep 20, 2016

mportuga reviewed Oct 18, 2016

View reviewed changes

mportuga added needs: decision and removed waiting-response labels Nov 7, 2016

mportuga added the Looks Good To Me label Nov 7, 2016

mportuga added waiting-response and removed Looks Good To Me labels Nov 16, 2016

mportuga closed this Dec 5, 2016

mportuga reopened this Aug 5, 2018

mportuga closed this Aug 6, 2018

Southpaw17 reviewed Dec 17, 2018

View reviewed changes

mportuga mentioned this pull request Jan 3, 2019

Expandable grid display issues when rows are expanded #4508

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fixed scrollPercent calculation for GridRenderContainer and InfiniteScroll Feature #5395

fixed scrollPercent calculation for GridRenderContainer and InfiniteScroll Feature #5395

vance commented May 10, 2016

swalters Jun 1, 2016

mportuga Oct 18, 2016

vance Oct 27, 2016

jrthib commented Jul 17, 2016

hexaclock commented Jul 25, 2016

vance commented Aug 9, 2016 •

edited

Loading

vance commented Aug 9, 2016 •

edited

Loading

mportuga commented Sep 20, 2016

vance commented Sep 21, 2016

cs45977 commented Oct 3, 2016

vance commented Oct 3, 2016 •

edited

Loading

cs45977 commented Oct 5, 2016

vance commented Oct 5, 2016 •

edited

Loading

cs45977 commented Oct 15, 2016

mportuga Oct 18, 2016

mportuga Oct 18, 2016

vance Oct 27, 2016

mportuga Oct 18, 2016

mportuga Oct 18, 2016

ionutb commented Nov 2, 2016

mportuga commented Nov 7, 2016

mportuga commented Nov 16, 2016

vance commented Nov 21, 2016

mportuga commented Dec 5, 2016

BPoorey commented Mar 7, 2017

nonongodup commented Aug 2, 2018 •

edited

Loading

mportuga commented Aug 5, 2018

mportuga commented Aug 6, 2018

nonongodup commented Aug 6, 2018

mportuga commented Aug 6, 2018

nonongodup commented Nov 20, 2018

Southpaw17 Dec 17, 2018

mportuga Jan 3, 2019


		self.updateViewableColumnRange(newRange);
		// Define a max row index that we can't scroll past

fixed scrollPercent calculation for GridRenderContainer and InfiniteScroll Feature #5395

fixed scrollPercent calculation for GridRenderContainer and InfiniteScroll Feature #5395

Conversation

vance commented May 10, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrthib commented Jul 17, 2016

hexaclock commented Jul 25, 2016

vance commented Aug 9, 2016 • edited Loading

vance commented Aug 9, 2016 • edited Loading

mportuga commented Sep 20, 2016

vance commented Sep 21, 2016

cs45977 commented Oct 3, 2016

vance commented Oct 3, 2016 • edited Loading

cs45977 commented Oct 5, 2016

vance commented Oct 5, 2016 • edited Loading

cs45977 commented Oct 15, 2016

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

ionutb commented Nov 2, 2016

mportuga commented Nov 7, 2016

mportuga commented Nov 16, 2016

vance commented Nov 21, 2016

mportuga commented Dec 5, 2016

BPoorey commented Mar 7, 2017

nonongodup commented Aug 2, 2018 • edited Loading

mportuga commented Aug 5, 2018

mportuga commented Aug 6, 2018

nonongodup commented Aug 6, 2018

mportuga commented Aug 6, 2018

nonongodup commented Nov 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vance commented Aug 9, 2016 •

edited

Loading

vance commented Aug 9, 2016 •

edited

Loading

vance commented Oct 3, 2016 •

edited

Loading

vance commented Oct 5, 2016 •

edited

Loading

nonongodup commented Aug 2, 2018 •

edited

Loading