marklieberman / downloadstar

Download all items in a webpage that match a pattern
GNU General Public License v3.0
90 stars 16 forks source link

Use `ng-cloak` for popup window until view is ready #53

Closed OkanEsen closed 6 years ago

OkanEsen commented 6 years ago

Based on the suggestion from @sokolow here: https://github.com/marklieberman/downloadstar/issues/49#issuecomment-406805255.

Wrapping the whole popup container with ng-cloak produced the best outcome for me. It takes around 1-2 seconds until the view comes up (for me at least) but I think instead of displaying a bogus window, this is the best approach.

OkanEsen commented 6 years ago

I just noticed, that wrapping the whole container seems rather radical to be honest. Maybe we should wrap all instances of {{ ... }} where this is causing issues?

Whats are your guys opinion on this @sokolow @marklieberman ?

marklieberman commented 6 years ago

The popup dimensions are determined by Firefox when it is first displayed, and there is a limited window to resize the content before the popup dimensions are selected. Cloaking the whole UI isn't the optimal solution here. The UI can be ready in a few milliseconds, it is compiling the ng-repeat with media items that is likely slow. Try deferring the start of scraping longer before cloaking elements.

OkanEsen commented 6 years ago

I added a simple console.log after the controls are loaded just to make sure, that the scraping is causing the slowdown and as you can see in the video, the cause seems to be that angular takes some time to render the UI.

Link to video: https://mnemo.okanesen.com/PVg2/downloadstar-bogus-popup.mov

I trimmed the video a bit where you can see the issue, as it doesn't happen every time.

marklieberman commented 6 years ago

I'll bet that that same test with https://github.com/marklieberman/downloadstar/blob/master/src/popup/popup.js#L837 commented out and the UI appears immediately. Its probably hanging on compiling the ng-repeat.

OkanEsen commented 6 years ago

This was actually the version where I put L837 inside a setTimeout but I changed it up and commented that line out and you can see the same result as before.

Link to video: https://mnemo.okanesen.com/8EDI/downloadstar-bogus-popup2.mov

marklieberman commented 6 years ago

Okay, I believe it isn't the scraping now.

I just opened the popup a whole bunch of times, like 50+ times in a row and maybe once I was able to see a bit of template before it compiled. Suggests to me that maybe angular.min.js is not being served fast enough.

In that case ng-cloak is probably the way to go. Just need to be careful not to cloak anything that is gonna mess with the popup sizing.

OkanEsen commented 6 years ago

I'm going to experiment with the ng-cloak approach on different places, instead of throwing it onto the whole popup.

OkanEsen commented 6 years ago

I just tested it by applying it to different places and the outcome was basically the same, so applying it to the whole container should actually suffice.

Below are two GIFs showing the behaviour after the changes: Popup: downloadstar-cloak-popup Overflow: downloadstar-cloak-overflow-menu