delite/Scrollable mixin

[AdrianVasiliu]

Contains:

• A mixin which adds scrolling capabilities to a widget, based on overflow:scroll.
• Intern unit tests.

A concrete widget (ScrollableContainer) using this mixin will be submitted to deliteful.

[AdrianVasiliu]

I've amended my commit (commit --amend) to fix one of the issues without realizing this will have the effect that Bill's valuable comments are no longer visible here. Instead, they can be seen here: AdrianVasiliu@97c2bd8 .
Starting from now I'll update my PR using a distinct commit.

[cjolif]

are there anymore comments? If not, maybe this is time to merge this?

[wkeese]

Seems like this should extend Widget too, like CssState and FormWidget. Is there some reason not to? It's referencing destroy() which is a method in Widget.

[cjolif]

I guess the idea was that the widget using this mixin would be the one inheriting from Widget not the mixin itself. I never remember what is the exact practice in that matter from the cases you are mentioning it seems to be the opposite. I guess that it should then indeed be modified. If that practice is not written down somewhere I guess it should be because it is at least for me not as obvious as it may sound.

[AdrianVasiliu]

I guess the idea was that the widget using this mixin would be the one inheriting from Widget not the mixin itself.

Yes, that was the idea. As Christophe said, if this is not the good practice, the explicit inheritance should be added.

[wkeese]

I have it written down in some google docs as my intention for delite. Not sure where you want to write it? In any case it's standard OOP practice to extend whatever classes you are depending on.

[cjolif]

I guess your google doc is fine, can you please share it again as I don't find it anymore?

[wkeese]

https://docs.google.com/document/d/1Ee4XL4TtTxojtkgRRetm4uIMAmiazH1CrA5Q6FKTlrM, specifically the part that says:

Make subclasses rather than mixins. For example, make Templated extend the Widget class, so that Templated subclasses classes (like Dialog) can just extend Templated, without mentioning Widget. I thought this wasn't possible due to "multiple inheritance issues", but it turns out that it works with dojo.declare() (not sure about ComposeJS) and the behavior is well defined by C3MRO.

[AdrianVasiliu]

From that I assume there's no technical issue with, say, a concrete widget extending more than one mixin itself extending Widget (for instance, Widget's implementation of lifecycle methods wouldn't be called more than once),
But I'm still wondering about the OOP semantic: by extending Widget, the mixin would clame being a Widget, isn't it? Since the class/module name doesn't contain anymore the word "mixin", we'd just count on the doc to clarify for the user that this isn't a concrete widget...Or maybe I'm missing something?

[wkeese]

From that I assume there's no technical issue with, say, a concrete widget extending more than one mixin itself extending Widget (for instance, Widget's implementation of lifecycle methods wouldn't be called more than once)

Right, C3MRO handles this.

But I'm still wondering about the OOP semantic: by extending Widget, the mixin would clame being a Widget, isn't it? Since the class/module name doesn't contain anymore the word "mixin"

You could also argue that Widget falsely claims that it's a widget, because it's not called WidgetBase or WidgetMixin. But we went round on naming classes, discussing various pros and cons (like excessive typing) and ended up with https://docs.google.com/document/d/1qjU9hm1HUnZIv051yC01VS81EmciA27GuqOkQPXmY3E, specifically the part that says:

Public classes and that must not be instantiated because they are just parent classes for implementations SHOULD be suffixed by Base (e.g. CalendarBase) when they need to differentiate themselves from the actual user accessible implementation (e.g. Calendar).

[AdrianVasiliu]

Well, I wasn't "fighting" against the agreed naming compromise, I was just pointing additional confusion if mixins extend Widget, given that their naming pattern doesn't convey the mixin semantic anymore.
In other words, since most things are a matter of pros and cons (as you said for the naming choice), I was wondering if widget mixins not extending Widget while using Widget API wouldn't be a balanced compromise between pros and cons.

[wkeese]

I was just pointing additional confusion if mixins extend Widget, given that their naming pattern doesn't convey the mixin semantic anymore.

But the naming pattern doesn't change depending on whether or not you extend Widget. Either way it's called Scrollable. Note the next line item in the google doc:

Mixins SHOULD not be postfixed by Mixin (ala dgrid or dtreemap, so Selection not SelectionMixin for example)

[wkeese]

I looked at it again briefly and added two more comments; I didn't see anything troubling beyond that.

[cjolif]

@wkeese @AdrianVasiliu being absent for a while what about modifying what you think should be modified and committing? This would allow to make some progress as others are needing this?

[cjolif]

merged in b7d4b33 (with the two additional fixes suggested by Bill)

[asudoh]

Just thought that this can be done with vanilla JS code…? (I don’t see any specific feature in domStyle.set() used here)

[cjolif]

Indeed. I guess we should have a clearer policy on whether we use domStyle or not and in which cases and once we have it apply it across the code.

[pruzand]

I created an issue to discuss this ibm-js/sdk#3

[asudoh]

Cool, thank you for doing this!