gemini-scrollbar icon indicating copy to clipboard operation
gemini-scrollbar copied to clipboard

remove / separeted _viewElement

Open noeldelgado opened this issue 7 years ago • 4 comments

When native scrollbars are supported and an onResize callback is passed, we need to create the resize trigger object for the event to be fired but we do not need to create a separeted _viewElement.

The way gemini works is that it will use the native scrollbars if the OS supports “overlay-scrollbars” (which means that the element you pass on the configuration should be scrollable by default overflow: auto|scroll).

noeldelgado avatar Jul 26 '16 20:07 noeldelgado

@richvdh can you please review this? I am asking to make sure this change does not break something on your side.

I just checked and can confirm that on this case if (DONT_CREATE_GEMINI && this.onResize) we don't need to move nodes, create, neither add class names to elements, the scrolling will still work as expected and the onResize callback will still be called.. in fact this was the line that makes me notice the issue 106: addClass(this.element, [CLASSNAMES.element]); because that one is preventing the native scroll behaviour (when “overlay-scrollbars” are supported natively and an onResize callback is passed).

noeldelgado avatar Jul 26 '16 20:07 noeldelgado

@noeldelgado: it's hard for me to test this, because my OS doesn't support overlay-scrollbars, but my thinking is as follows:

onResize is supposed to be called when the external size (offsetWidth/offsetHeight) of the div changes. However, if you put the resize-trigger inside the div, then it will match the internal size (scrollWidth/scrollHeight) of the div. I think you won't see this on your example, because the internal and external size of #example-7 are always the same.

So even if the OS supports native scrollbars, you need an outer div which doesn't scroll, which contains both the resize-trigger and the viewElement which does the scrolling.

I guess the inner viewElement could use the native scroll behaviour if DONT_CREATE_GEMINI is false. I think I made it use the gemini scrollbar in this case because it was easier for me to get right without being able to test on OS X.

(In fact, as things are here, the resize-trigger won't match the size at all, because the width==height==100% trick only works if the parent has position: relative, but that is probably solved more easily.)

richvdh avatar Jul 27 '16 09:07 richvdh

Thank you @richvdh, I’ve been thinking this for a while today and this is what I’m planning to do:

  • Keep v1.x.x on a new branch (v1 for instance) for further additions
  • Revert the resizer-trigger feature and publish it as v2.0.0

This way we can have both versions and see how it goes, as you may know npm allows us to publish previous versions, so updating v1/v2 shouldn’t be a problem.

Also, users can choose which version fits better for their needs (the auto-update-call or calling the update method manually).

I like the idea of having the resize-trigger and the onResize hook, that solves some specific use-cases, but being honest I haven’t really needed them yet, the update method was designed to be called on demand so my programs followed that approach and after upgrading I noticed it ends up calling the update method several times (because I am calling it manually too), so I need to update all my programs if I upgrade them to use the latest version of gemini.

That’s basically why I am considering on keeping both versions.

What do you think?

noeldelgado avatar Jul 27 '16 17:07 noeldelgado

Well, it's up to you, of course, but personally, I would be wary about ending up maintaining two branches of the code. Also, most people won't take the time to check the different versions of the project and will always use the "latest".

If you're proposing to make the resize-trigger optional, then I think it would make more sense to make it configurable with a parameter to the constructor.

richvdh avatar Jul 27 '16 21:07 richvdh