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

[cla] Convert to AMD #82

Closed
wants to merge 4 commits into from

Conversation

Projects
None yet
3 participants
@agavrilov
Copy link

commented Mar 21, 2014

Converted to AMD some modules from dojox/image and dojox/widget

agavrilov added some commits Mar 21, 2014

Convert to AMD
Convert to AMD dojox/image/Gallery, dojox/image/SlideShow and
dojox/image/ThumbnailPicker
Convert to AMD
Convert to AMD dojox/widget/RollingList
@dylans

This comment has been minimized.

Copy link
Member

commented Mar 21, 2014

HI @agavrilov thanks for your contribution! Can you please tell us who you are, so we can check to see if you have a CLA on file ( https://github.com/dojo/dojox/blob/master/CONTRIBUTING.md#contributor-license-agreement ). Also, if you could reference a corresponding bug in dojo as noted in the instructions at https://github.com/dojo/dojox/blob/master/CONTRIBUTING.md#6-issue-a-pull-request , that will help us make sure it gets resolved to our next release, updated in the documentation, etc. Thanks, and let me know if you have any questions.

@agavrilov

This comment has been minimized.

Copy link
Author

commented Mar 21, 2014

I have filed CLA on dojofoundation.org. It is from Alexander Gavrilov (me).

As for the bug, I don't think there is a corresponding bug in the bug tracker.

@dylans

This comment has been minimized.

Copy link
Member

commented Mar 21, 2014

Ok, added to https://bugs.dojotoolkit.org/ticket/17801 ... we'll review as soon as time permits.

@dylans dylans added this to the 1.10 milestone Apr 9, 2014

@dylans dylans changed the title Convert to AMD [cla] Convert to AMD Apr 9, 2014

@@ -1,5 +1,11 @@
dojo.provide("dojox.image.Gallery");
dojo.experimental("dojox.image.Gallery");
define("dojox/image/Gallery",[

This comment has been minimized.

Copy link
@brandonpayton

brandonpayton May 5, 2014

Member

The module ID should not be passed as the first parameter to define. The module ID is determined by the loader at runtime. This should be adjusted throughout this PR.

One exception is define calls in layers created by Dojo's build system. Build layers pass the module ID as the first argument to define because the modules defined in the layer are no longer defined in their own .js files with locations that correspond to their module IDs.


dojo.declare("dojox.image.Gallery",
[dijit._Widget, dijit._Templated],
return dojo.declare("dojox.image.Gallery",

This comment has been minimized.

Copy link
@brandonpayton

brandonpayton May 5, 2014

Member

There are some lingering calls to dojo.declare that should be replaced with use of dojo/_base/declare.

"dojo/i18n!dijit/nls/common",
"dijit/_base/focus", // dijit.getFocus()
"dijit/focus" // dijit.focus()
], function(dojo,declare,i18n,windowUtils,metrics,_Contained,_Container,_TemplatedMixin,_WidgetsInTemplateMixin,_Widget,Button,_LayoutWidget,ContentPane,Menu,MenuItem,templateRollingList) {

This comment has been minimized.

Copy link
@brandonpayton

brandonpayton May 5, 2014

Member

windowUtils should be renamed to winUtils because that is how most of the toolkit references dojo/window.

@brandonpayton

This comment has been minimized.

Copy link
Member

commented May 5, 2014

Hi @agavrilov, I noted a few things that need to be fixed in this PR. Once those are addressed, I believe this will be ready to merge.

One more thing I noticed is that some the tests for these modules need to be updated to AMD as well. If you want to update them, go ahead. Otherwise, I will look at updating them when we merge this PR.

Thanks!

agavrilov added some commits May 5, 2014

Remove module ID. Use 'dojo/_base/declare'
Remove module ID as a first parameter of 'define'. Use
'dojo/_base/declare' instead of dojo.declare
@agavrilov

This comment has been minimized.

Copy link
Author

commented May 5, 2014

I have made following changes:

  • Removed module ID from 'define' calls.
  • Use 'dojo/_base/declare' now.
  • 'windowUtils' renamed to 'winUtils' according to suggestion.
dojo.provide("dojox.image.Gallery");
dojo.experimental("dojox.image.Gallery");
define([
"dojo",

This comment has been minimized.

Copy link
@dylans

dylans May 20, 2014

Member

We should not be relying on top level dojo modules any more, but instead the smaller dependencies

"dojo/_base/declare",
"dijit/_TemplatedMixin",
"dijit/_Widget",
"dojox/image/SlideShow",

This comment has been minimized.

Copy link
@dylans

dylans May 20, 2014

Member

Any dojox dependencies should use relative mid, not absolute, e.g. "./SlideShow"

@@ -112,7 +113,7 @@ dojo.declare("dojox.image.Gallery",
});
this._centerChildren();
},

setDataStore: function(dataStore, request, /*optional*/paramNames){

This comment has been minimized.

Copy link
@dylans

dylans May 20, 2014

Member

Ideally this would be using get/set rather than this approach. Also, is this using dojo/data or dojo/store?

"dojo",
"dojo/_base/declare",
"dijit/_TemplatedMixin",
"dijit/_Widget",

This comment has been minimized.

Copy link
@dylans

dylans May 20, 2014

Member

Any reason to not use _WidgetBase here?

_calcNavDimensions: function() {
// summary:
// Calculates the dimensions of the navigation controls
dojo.style(this.navNode, "position", "absolute");


//Place the navigation controls far off screen
dojo.style(this.navNode, "top", "-10000px");

This comment has been minimized.

Copy link
@dylans

dylans May 20, 2014

Member

these should be relying on dojo/dom-style rather than dojo

@@ -535,21 +540,21 @@ dojo.declare("dojox.image.SlideShow",
dijit.byId(id).showNextImage(true);

This comment has been minimized.

Copy link
@dylans

dylans May 20, 2014

Member

Should be dijit/registry rather than global dijit

@@ -496,7 +501,7 @@ dojo.declare("dojox.image.SlideShow",

_this.images[index] = div;
dojo.attr(img, "src", url);

This comment has been minimized.

Copy link
@dylans

dylans May 20, 2014

Member

Should use dojo/dom-attr rather than dojo global

@@ -456,7 +461,7 @@ dojo.declare("dojox.image.SlideShow",
var store = this.imageStore;
var loadIt = function(item){
var url = _this.imageStore.getValue(item, _this.imageLargeAttr);

var img = new Image(); // when creating img with "createElement" IE doesnt has width and height, so use the Image object
var div = dojo.create("div", {

This comment has been minimized.

Copy link
@dylans

dylans May 20, 2014

Member

Should use dojo/dom-construct rather than dojo

@dylans

This comment has been minimized.

Copy link
Member

commented May 20, 2014

I appreciate the efforts on this one. There are enough pre-AMD patterns still in this module that I would like to see fixed before this lands in master. I'm going to move this to 1.11.

@dylans

This comment has been minimized.

Copy link
Member

commented Oct 5, 2014

@agavrilov do you plan to finish working on this one, based on the earlier feedback?

@dylans

This comment has been minimized.

Copy link
Member

commented Nov 2, 2014

I will submit a pull request to replace this one when I'm finished, work in progress at dylans@45aadee

Closing this out as there's a lot of additional work needed to make this work.

@dylans dylans closed this Nov 2, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.