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

Event based reinitialising - Resize Sensor #361

Merged
merged 1 commit into from
Apr 28, 2018
Merged

Event based reinitialising - Resize Sensor #361

merged 1 commit into from
Apr 28, 2018

Conversation

ashley-hunter
Copy link
Contributor

Brought in the most recent resize sensor code. Continuing the discussion from: #341

@illuusio
Copy link
Collaborator

Thank you for the contribution. I test this and try to get it in

@illuusio illuusio added Echament Need testing Help Needed If you have spare time you can test these labels Apr 19, 2018
resizeElement.style.cssText = 'position: absolute; left: 0; top: 0; right: 0; bottom: 0; overflow: scroll; z-index: -1; visibility: hidden;';
resizeGrowElement.style.cssText = 'position: absolute; left: 0; top: 0; right: 0; bottom: 0; overflow: scroll; z-index: -1; visibility: hidden;';
resizeShrinkElement.style.cssText = 'position: absolute; left: 0; top: 0; right: 0; bottom: 0; overflow: scroll; z-index: -1; visibility: hidden;';

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where these CSS styles come from? Are they some tasked knownledge wizardy that I have not access to?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The resize detection is based on this idea: (https://github.com/marcj/css-element-queries/blob/master/src/ResizeSensor.js)[https://github.com/marcj/css-element-queries/blob/master/src/ResizeSensor.js].

Essentially the divs are positioned in a way that if there is any change in size it would cause a scroll event to fire, which we can then use to detect when to reinitialise. This particular css is tasked with creating the divs so they fill the entire area without being visible or having any effect on the content the user places in it.


resizeGrowChildElement.style.width = resizeGrowElement.offsetWidth + 10 + 'px';
resizeGrowChildElement.style.height = resizeGrowElement.offsetHeight + 10 + 'px';

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why 10 px?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't necessarily need to be 10px, we just need to increase it so it is bigger in size that its parent container.

// update sizes initially
updateSizes();
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does this prevent getting into infinite resize loop? Is timer for that?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Essentially once a resize has occurred the sensor divs are likely no longer the size they need to trigger a scroll event (detect a resize). So we need to update their sizes. Setting the size of these elements does not trigger a resize event again as it will only be triggered when the parent container changes in size.

The timer isn't for that, it is just there as an additional feature, where a user may wish to delay the reinit after a resize occurs.

I hope that makes sense, it is quite difficult to explain.

@gitbreaker222
Copy link

gitbreaker222 commented Apr 20, 2018

i have set up a demo - unfortunately it seems not to resolve the issue I addressed in the prior discusson 😟

how to test

reinitialize with interval: https://jsfiddle.net/pa5ugd0v/6/
reinitialize with event (this PR): https://jsfiddle.net/pcwudrmc/12519/

  • inspect the container div with the class .scroll-pane
  • --> the immediate child .jspContainer should NOT refresh infinite while idle

However both do

background problem

The content of the .scroll-pane has more width (20em) than the container (19em). This seems to cause the infinite reinitializing and it seems to remain with the changes here.

tested in latest Chromium. When changing the fiddle, clear app cache stuff or test in private window

2018-04-20_performance-drain-demo
edit: I hope I've did everything right with the demo setup. This is my first real contribution to open source 😁

@ashley-hunter
Copy link
Contributor Author

Hi @gitbreaker222, thanks for the examples! I notice one thing in the second example that may need updated to get the event based resizing to work. In the event based example the settings still use autoReinitialise: true. To take advantage of the changes in this PR try changing it to resizeSensor: true instead.

I didn't replace the autoReinitialise setting because there are a few edge cases where the event based resizing wouldn't suit. Hopefully you'll notice a good improvement after you make that change! If not let me know and I can look into it further 👍

@gitbreaker222
Copy link

gitbreaker222 commented Apr 21, 2018

thanks for the hint @ashley-hunter 👍
I've updated the fiddle with the resizeSensor and it solves the problem 😌
https://jsfiddle.net/pcwudrmc/12831/

  • the panel adapts when the width of the window changes
  • no endless relayouts

(demo jScrollPanel does not scroll, because I did not include mousewheel.js. Focus should be on the automatic resize.)

@illuusio
Copy link
Collaborator

illuusio commented Apr 22, 2018

Ok I test this once more and try to merge this. As this is in and more npm build logic merge I think I'll make release candidate after that to have more testing. Should there be changes to examples because those jsFiddle examples? I just looked through vert roughly and thanks @gitbreaker222 you are doing fine (as you test this for me).

@gitbreaker222
Copy link

I have used jsFiddle, because it was more easy for me to compare intervall/event resizing and analyse the performance drain.

My demo is basically just a mini version of the new example page examples/resize_sensor.html.

Aside from that, examples/auto_reinitialise.html won't advertise using autoReinitialise anymore and links to examples/resize_sensor.html instead.

The list in examples/index.html also has a new entry for the sensor, as well as the documentation in examples/settings.html

@illuusio
Copy link
Collaborator

@gitbreaker222 Thank you for the explanation. Yes I noticed that all there is new example and settings update which is 👍 for getting this in.

@illuusio illuusio merged commit 153a5ef into vitch:master Apr 28, 2018
@illuusio
Copy link
Collaborator

Merged thanks.

@ashley-hunter ashley-hunter deleted the resize-sensor branch April 30, 2018 10:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Echament Help Needed If you have spare time you can test these Need testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants