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

remove / separeted _viewElement #33

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

noeldelgado
Copy link
Owner

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
Copy link
Owner Author

noeldelgado commented Jul 26, 2016

@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).

@richvdh
Copy link
Contributor

richvdh commented Jul 27, 2016

@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.)

@noeldelgado
Copy link
Owner Author

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?

@richvdh
Copy link
Contributor

richvdh commented Jul 27, 2016

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants