Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bug: CollectionRepeat wrong event 'scroll.resize' #3384

Closed
dreamershl opened this issue Mar 23, 2015 · 3 comments
Closed

bug: CollectionRepeat wrong event 'scroll.resize' #3384

dreamershl opened this issue Mar 23, 2015 · 3 comments

Comments

@dreamershl
Copy link

Type: bug

Platform: all

The following code hook the wrong event 'scroll.resize'. The JQuery event name is event+namespace. So this event will be interpreted as 'scroll' event, which will be triggered for every scrolling. Because the scroll view will trigger the scroll event through the "triggerScrollEvent()".

The consequence is the "CollectionRepeat" will always be forced to refresh the layout for each scrolling. This will waste the performance.

// Dimensions are refreshed on resize or data change.
scrollCtrl.$element.on('scroll.resize', refreshDimensions);</span>

@perrygovier
Copy link
Contributor

Hey @dreamershl, That would be a big problem. Can you put together a codepen demonstrating that refreshDimensions() is indeed firing every scroll event?

@perrygovier perrygovier added the needs: reply the issue needs a response from the user label Mar 24, 2015
@Fayozjon
Copy link

Perry

http://wenxuebu.com/forex-dollar

24.03.2015, 19:30, "Perry Govier" notifications@github.com:Hey , That would be a big problem. Can you put together a codepen demonstrating that refreshDimensions() is indeed firing every scroll event?

—Reply to this email directly or .

@Fayozjon
Copy link

Shen

http://interior-line.ru/realizovato

23.03.2015, 19:45, "Shen liang" notifications@github.com:Type: bug

Platform: all

The following code hook the wrong event 'scroll.resize'. The JQuery event name is event+namespace. So this event will be interpreted as 'scroll' event, which will be triggered for every scrolling. Because the scroll view will trigger the scroll event through the "triggerScrollEvent()".

The consequence is the "CollectionRepeat" will always be forced to refresh the layout for each scrolling. This will waste the performance.

// Dimensions are refreshed on resize or data change.
scrollCtrl.$element.on('scroll.resize', refreshDimensions);

—Reply to this email directly or .

@dreamershl
Copy link
Author

Hi, because of my unfamiliarity to the codepen, I just paste the screen capture of my current environment for your reference. 1 is the detail call stack analysis, another one is my working environment for your reference.

When scrolling the list, the following is the key call path

self.__publish(scrollLeft, scrollTop, level); 
           -> self.triggerScrollEvent();
                       ->refreshDimensions();
                                ->getRepeatManager().refreshLayout();

All because of this event triggering "ionic.trigger('scroll', {" in the triggerScrollEvent(). It triggers a global scroll event (no namespace as suffix). So the current event handler "scrollCtrl.$element.on('scroll.resize', refreshDimensions)" will be invoked.

If correct the event name from "scroll.resize" to "resize.scroll". The "refreshDimensions" will be only invoked when the element is resized. Just FYI, there are 3 places to be corrected.

Last one, may I request a same small enhancement in this new version again? Please export the current shown items. If it does, the application get chance to dynamically load the data. This is extremely helpful on the real time apps performance such as stock price updates.

To simplify the export,
1: instead of the closure, put the 'render()' as the method of the "RepeatController"

this.render=function(forceRerender) { 

2: return the "itemsShownMap" from the "this.render"

Then the application get chance to know the current visible items exactly!

Call stacks:
image

My working environment:
image

@Ionitron Ionitron removed the needs: reply the issue needs a response from the user label Mar 25, 2015
@ajoslin
Copy link
Contributor

ajoslin commented Apr 15, 2015

Good catch. jQuery will clobber our DOM events with . in them, noted.

@ionitron-bot ionitron-bot bot locked and limited conversation to collaborators Sep 7, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants