delite/Scrollable mixin #79
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,242 @@ | ||
| define([ | ||
| "dcl/dcl", | ||
| "dojo/dom", | ||
| "dojo/dom-style", | ||
| "dojo/dom-class", | ||
| "dojo/_base/fx", | ||
| "dojo/fx/easing", | ||
| "delite/Invalidating", | ||
| "delite/themes/load!./Scrollable/themes/{{theme}}/Scrollable_css" | ||
| ], function (dcl, dom, domStyle, domClass, baseFx, easing, Invalidating) { | ||
|
|
||
| // module: | ||
| // delite/Scrollable | ||
|
|
||
| return dcl(Invalidating, { | ||
| // summary: | ||
| // A mixin which adds scrolling capabilities to a widget. | ||
| // description: | ||
| // When mixed into a widget, this mixin adds to it scrolling capabilities | ||
| // based on the overflow: scroll CSS property. | ||
| // By default, the scrolling capabilities are added to the widget | ||
| // node itself. The host widget can chose the node thanks to the property | ||
| // 'scrollableNode'. | ||
| // During interactive or programmatic scrolling, native "scroll" | ||
| // events are emitted, and can be listen as follows (here, | ||
| // 'scrollWidget' is the widget into which this mixin is mixed): | ||
| // | scrollWidget.on("scroll", function () { | ||
| // | ... | ||
| // | } | ||
| // For widgets that customize the 'scrollableNode' property, | ||
| // the events should be listen on widget.scrollableNode. | ||
| // TODO: improve the doc. | ||
|
|
||
| // TODO: optional styling of the scrollbar for browsers which by default do not provide | ||
| // a scroll indicator. | ||
|
|
||
| // scrollDirection: String | ||
| // The direction of the interactive scroll. Possible values are: | ||
| // "vertical", "horizontal", "both, and "none". The default value is "vertical". | ||
| // Note that scrolling programmatically using scrollTo() is | ||
| // possible on both horizontal and vertical directions independently | ||
| // on the value of scrollDirection. | ||
| scrollDirection: "vertical", | ||
|
|
||
| // scrollableNode: [readonly] DomNode | ||
| // Designates the descendant node of this widget which is made scrollable. | ||
| // The default value is 'null'. If not set, defaults to this widget | ||
| // itself ('this'). | ||
| // Note that this property can be set only at construction time, as a | ||
| // constructor argument or in markup as a property of the widget into | ||
| // which this class is mixed. | ||
| scrollableNode: null, | ||
|
|
||
| preCreate: function () { | ||
| this.addInvalidatingProperties("scrollDirection"); | ||
| }, | ||
|
|
||
| refreshRendering: dcl.after(function () { | ||
| if (!this.scrollableNode) { | ||
| this.scrollableNode = this; // If unspecified, defaults to 'this'. | ||
| } | ||
|
|
||
| domClass.toggle(this.scrollableNode, "d-scrollable", this.scrollDirection !== "none"); | ||
|
|
||
| dom.setSelectable(this.scrollableNode, false); | ||
|
|
||
| domStyle.set(this.scrollableNode, "overflowX", | ||
|
There was a problem hiding this comment. Just thought that this can be done with vanilla JS code…? (I don’t see any specific feature in There was a problem hiding this comment. 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. There was a problem hiding this comment. I created an issue to discuss this ibm-js/sdk#3 There was a problem hiding this comment. Cool, thank you for doing this! |
||
| /^(both|horizontal)$/.test(this.scrollDirection) ? "scroll" : ""); | ||
| domStyle.set(this.scrollableNode, "overflowY", | ||
| /^(both|vertical)$/.test(this.scrollDirection) ? "scroll" : ""); | ||
| }), | ||
|
|
||
| buildRendering: dcl.after(function () { | ||
| this.invalidateRendering(); | ||
| }), | ||
|
|
||
| destroy: function () { | ||
| this._stopAnimation(); | ||
| }, | ||
|
|
||
| isTopScroll: function () { | ||
| // summary: | ||
| // Returns true if container's scroll has reached the maximum at | ||
| // the top of the content. Returns false otherwise. | ||
| // example: | ||
| // | scrollContainer.on("scroll", function () { | ||
| // | if (scrollContainer.isTopScroll()) { | ||
| // | console.log("Scroll reached the maximum at the top"); | ||
| // | } | ||
| // | } | ||
| // returns: Boolean | ||
| return this.scrollableNode.scrollTop === 0; | ||
| }, | ||
|
|
||
| isBottomScroll: function () { | ||
| // summary: | ||
| // Returns true if container's scroll has reached the maximum at | ||
| // the bottom of the content. Returns false otherwise. | ||
| // example: | ||
| // | scrollContainer.on("scroll", function () { | ||
| // | if (scrollContainer.isBottomScroll()) { | ||
| // | console.log("Scroll reached the maximum at the bottom"); | ||
| // | } | ||
| // | } | ||
| // returns: Boolean | ||
| var scrollableNode = this.scrollableNode; | ||
| return scrollableNode.offsetHeight + scrollableNode.scrollTop >= | ||
| scrollableNode.scrollHeight; | ||
| }, | ||
|
|
||
| isLeftScroll: function () { | ||
| // summary: | ||
| // Returns true if container's scroll has reached the maximum at | ||
| // the left of the content. Returns false otherwise. | ||
| // example: | ||
| // | scrollContainer.on("scroll", function () { | ||
| // | if (scrollContainer.isLeftScroll()) { | ||
| // | console.log("Scroll reached the maximum at the left"); | ||
| // | } | ||
| // | } | ||
| // returns: Boolean | ||
| return this.scrollableNode.scrollLeft === 0; | ||
| }, | ||
|
|
||
| isRightScroll: function () { | ||
| // summary: | ||
| // Returns true if container's scroll has reached the maximum at | ||
| // the right of the content. Returns false otherwise. | ||
| // example: | ||
| // | scrollContainer.on("scroll", function () { | ||
| // | if (scrollContainer.isRightScroll()) { | ||
| // | console.log("Scroll reached the maximum at the right"); | ||
| // | } | ||
| // | } | ||
| // returns: Boolean | ||
| var scrollableNode = this.scrollableNode; | ||
| return scrollableNode.offsetWidth + scrollableNode.scrollLeft >= scrollableNode.scrollWidth; | ||
| }, | ||
|
|
||
| getCurrentScroll: function () { | ||
| // summary: | ||
| // Returns the current amount of scroll, as an object with x and y properties | ||
| // for the horizontal and vertical scroll amount. | ||
| // This is a convenience method and it is not supposed to be overridden. | ||
| // returns: Object | ||
| return {x: this.scrollableNode.scrollLeft, y: this.scrollableNode.scrollTop}; | ||
| }, | ||
|
|
||
| scrollBy: function (by, duration) { | ||
| // summary: | ||
| // Scrolls by the given amount. | ||
| // by: | ||
| // The scroll amount. An object with x and/or y properties, for example | ||
| // {x:0, y:-5} or {y:-29}. | ||
| // duration: | ||
| // Duration of scrolling animation in milliseconds. If 0 or unspecified, | ||
| // scrolls without animation. | ||
| var to = {}; | ||
| if (by.x !== undefined) { | ||
| to.x = this.scrollableNode.scrollLeft + by.x; | ||
| } | ||
| if (by.y !== undefined) { | ||
| to.y = this.scrollableNode.scrollTop + by.y; | ||
| } | ||
| this.scrollTo(to, duration); | ||
| }, | ||
|
|
||
| scrollTo: function (to, /*Number?*/duration) { | ||
| // summary: | ||
| // Scrolls to the given position. | ||
| // to: | ||
| // The scroll destination position. An object with x and/or y properties, | ||
| // for example {x:0, y:-5} or {y:-29}. | ||
| // duration: | ||
| // Duration of scrolling animation in milliseconds. If 0 or unspecified, | ||
| // scrolls without animation. | ||
| var self = this; | ||
| var scrollableNode = this.scrollableNode; | ||
| var from; | ||
| var animation, anim, Curve; | ||
|
There was a problem hiding this comment. Is there some reason to declare all the variables at the top of the function? Some jshint setting? I would just declare them where they are actually defined, for example I realize this might be a religious issue but I thought for most of our code we are doing it the opposite way from what you've done. There was a problem hiding this comment. They used to be declared where they are defined, and I changed it recently when I remembered having been told that declaring at use is misleading about the scope. So yes, it is for religious reasons but not my religion ;-) ... There was a problem hiding this comment. OK, the practice/religion in dojo and ibm-dojo is actually to declare where used, as implied by our .jshintrc, and as documented in https://github.com/csnover/dojo2-core#linting, specifically the line:
|
||
| this._stopAnimation(); | ||
| if (!duration || duration <= 0) { // shortcut | ||
| if (to.x !== undefined) { | ||
| scrollableNode.scrollLeft = to.x; | ||
| } | ||
| if (to.y !== undefined) { | ||
| scrollableNode.scrollTop = to.y; | ||
| } | ||
| } else { | ||
| from = { | ||
| x: to.x !== undefined ? scrollableNode.scrollLeft : undefined, | ||
| y: to.y !== undefined ? scrollableNode.scrollTop : undefined | ||
| }; | ||
| anim = function () { | ||
| // dojo/_base/fx._Line cannot be used for animating several | ||
| // properties at once (scrollTop and scrollLeft in our case). | ||
| // Hence, using instead a custom function: | ||
| Curve = function (/*int*/ start, /*int*/ end){ | ||
| this.start = start; | ||
| this.end = end; | ||
| }; | ||
| Curve.prototype.getValue = function (/*float*/ n){ | ||
| return { | ||
| x: ((to.x - from.x) * n) + from.x, | ||
| y: ((to.y - from.y) * n) + from.y | ||
| }; | ||
| }; | ||
| animation = new baseFx.Animation({ | ||
| beforeBegin: function () { | ||
| if (this.curve) { | ||
| delete this.curve; | ||
| } | ||
| animation.curve = new Curve(from, to); | ||
| }, | ||
| onAnimate: function (val) { | ||
| if (val.x !== undefined) { | ||
| scrollableNode.scrollLeft = val.x; | ||
| } | ||
| if (val.y !== undefined) { | ||
| scrollableNode.scrollTop = val.y; | ||
| } | ||
| }, | ||
| easing: easing.expoInOut, // TODO: IMPROVEME | ||
| duration: duration, | ||
| rate: 20 // TODO: IMPROVEME | ||
| }); | ||
| self._animation = animation; | ||
| return animation; // dojo/_base/fx/Animation | ||
| }; | ||
| anim().play(); | ||
| } | ||
| }, | ||
|
|
||
| _stopAnimation: function () { | ||
| // summary: | ||
| // Stops the scrolling animation if it is currently playing. | ||
| if (this._animation && this._animation.status() === "playing") { | ||
| this._animation.stop(); | ||
| } | ||
| } | ||
| }); | ||
| }); | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| .d-scrollable { | ||
| -webkit-overflow-scrolling: touch; | ||
| /* enable hardware acceleration */ | ||
| -webkit-transform: translate3d(0,0,0); | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| @import "../Scrollable_template"; |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| define(function(){ return '\ | ||
| .d-scrollable {\ | ||
| -webkit-overflow-scrolling: touch;\ | ||
| /* enable hardware acceleration */\ | ||
| -webkit-transform: translate3d(0, 0, 0);\ | ||
| }\ | ||
| '; } ); |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| @import "../Scrollable_template"; |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| define(function(){ return '\ | ||
| .d-scrollable {\ | ||
| -webkit-overflow-scrolling: touch;\ | ||
| /* enable hardware acceleration */\ | ||
| -webkit-transform: translate3d(0, 0, 0);\ | ||
| }\ | ||
| '; } ); |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| @import "../Scrollable_template"; |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| define(function(){ return '\ | ||
| .d-scrollable {\ | ||
| -webkit-overflow-scrolling: touch;\ | ||
| /* enable hardware acceleration */\ | ||
| -webkit-transform: translate3d(0, 0, 0);\ | ||
| }\ | ||
| '; } ); |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| @import "../Scrollable_template"; |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| define(function(){ return '\ | ||
| .d-scrollable {\ | ||
| -webkit-overflow-scrolling: touch;\ | ||
| /* enable hardware acceleration */\ | ||
| -webkit-transform: translate3d(0, 0, 0);\ | ||
| }\ | ||
| '; } ); |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| @import "../Scrollable_template"; |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| define(function(){ return '\ | ||
| .d-scrollable {\ | ||
| -webkit-overflow-scrolling: touch;\ | ||
| /* enable hardware acceleration */\ | ||
| -webkit-transform: translate3d(0, 0, 0);\ | ||
| }\ | ||
| '; } ); |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| @import "../Scrollable_template"; |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| define(function(){ return '\ | ||
| .d-scrollable {\ | ||
| -webkit-overflow-scrolling: touch;\ | ||
| /* enable hardware acceleration */\ | ||
| -webkit-transform: translate3d(0, 0, 0);\ | ||
| }\ | ||
| '; } ); |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that was the idea. As Christophe said, if this is not the good practice, the explicit inheritance should be added.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess your google doc is fine, can you please share it again as I don't find it anymore?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://docs.google.com/document/d/1Ee4XL4TtTxojtkgRRetm4uIMAmiazH1CrA5Q6FKTlrM, specifically the part that says:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, C3MRO handles this.
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:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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: