Skip to content
This repository was archived by the owner on May 23, 2025. It is now read-only.

fixed and improved JS scaffolding #32

Open
swey wants to merge 4 commits into
base: develop
Choose a base branch
from
Open

fixed and improved JS scaffolding #32

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
32ad302
fixed and improved JS scaffolding
swey Jan 17, 2017
d8ccf82
further improvement of the scaffolding (removed the name constant)
swey Jan 17, 2017
04d65e9
adjusted the instance's name to class' name
swey Jan 17, 2017
16a5113
adjusted the scaffolding gulp task to the new naming
swey Jan 17, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion gulp/scaffold/default.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ var taskName = 'scaffold',
src: './source/assets/js/helpers/estaticoapp.js',
insertionPoint: '/* autoinsertmodule */',
importInsertionPoint: '/* autoinsertmodulereference */',
insertionTemplate: 'this.modules.{{keyName}} = {{className}};\n ',
insertionTemplate: 'this.modules[{{className}}.name] = {{className}};\n ',
importInsertionTemplate: 'import {{className}} from \'{{modulePath}}\';\n'
},

Expand Down
2 changes: 1 addition & 1 deletion source/assets/js/helpers/module.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ class EstaticoModule {
* @param {object} options - The options passed as data attribute in the Module
*/
constructor($element, _defaultData, _defaultOptions, data, options) {
this.name = this.constructor.name.toLowerCase();
this.name = this.constructor.name;
Copy link
Copy Markdown
Member

@drowka drowka Jan 19, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might consider forcing the lowercase again. We usually use this.name for data-attributes matching and - quoting specs - these have to be lowercase.

The name of a custom data attribute in HTML begins with data-. It must contain only letters, numbers and the following characters: dash (-), dot (.), colon (:), underscore (_) -- but NOT any ASCII capital letters (A to Z).

Copy link
Copy Markdown
Contributor Author

@swey swey Jan 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No reason to convert the case to lower case. What will happen, you can see in the current master branch - sometimes everything breaks, because everyone is using a different case. (Yes, the master branch is broken at the moment.)

Also it is not possible to convert this outside an instance, e.g. getting a class' name in the right case. It only works after an instance is created and needs to be converted for each instance over and over agin. Please also see #33 where I found a quite clean solution for the module registration.

(In our current setup we convert original class names like "Image Gallery" to the file name "image_gallery" and class name "ImageGallery". By using the class name instead of an extra lowercase for the assignment, we don't need a third case.)

Copy link
Copy Markdown
Collaborator

@backflip backflip Jan 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@swey, so you would use [data-' + this.name.toLowerCase() + '="bla"'] and $domElement.data(this.name.toLowerCase()) instead?

Copy link
Copy Markdown
Contributor Author

@swey swey Jan 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, got it. Sorry, I only thought of data-init. (We use a different data binding, so I forgot about that.)

Don't have a good solution in mind right now. Will think about it

Copy link
Copy Markdown
Member

@orioltf orioltf Jan 25, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I definitely like the approach from #32 and #33. However the data- attributes management remains open, and the approach from @backflip ([data-' + this.name.toLowerCase() + '="bla"']) seems rather error prone. How about having this.name and this._name/this.dataName for these 2 purposes (I know, my naming skills are really bad… feel free to propose another one).

This has moreover impact on linting configuration and how we would have to specify defaults in head.js: we would need to use PascalCased object properties instead of "camelCased". Note that many times head.js is used as the configuration point for backend developers, so having proper naming conventions there is important. @backflip would it be fine for you to change linting rules to accept as well PascalCased object properties?

Copy link
Copy Markdown
Contributor Author

@swey swey Jan 25, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main problem is, that working with this.name is only working inside an instance. Which means that all properties must be set at runtime and for every instance again (which can cause a lot of extra operations and will block more memory). But well, of course we're only talking of quite simple and cheap string operations.

But I don't see a clean solution for your data binding besides adding a const name = 'module_in_yor_prefered_case'; before the class definition - very similar to the old version of estatico.

(In our team we use data-js-binding without the module name. But of course binding to multiple instances is not that clean. On the other hand it solved a lot of problems with nested modules for us.)

Copy link
Copy Markdown
Contributor Author

@swey swey Jan 25, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having a new idea on this. Coming back with it the next days :)


this.ui = {
$element
Expand Down
61 changes: 30 additions & 31 deletions source/modules/.scaffold/scaffold.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,59 +1,58 @@
/*!
import EstaticoModule from '../../assets/js/helpers/module';

/*
* {{originalName}}
*
* @author
* @copyright
*/
import EstaticoModule from '../../assets/js/helpers/module';

class {{className}} extends EstaticoModule {
export default class {{className}} extends EstaticoModule {
static events = {
Copy link
Copy Markdown
Collaborator

@backflip backflip Jan 17, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I certainly like the idea, could a stage 2 proposal be a bit too experimental to be used in production already? https://github.com/tc39/proposal-class-public-fields

Judging from https://github.com/babel/babel/search?q=static+property&type=Issues&utf8=%E2%9C%93, there might still be some cases where the transpiled result might not be the expected one.

Copy link
Copy Markdown
Contributor Author

@swey swey Jan 17, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did not see any major problems there, most problems are caused by wrong usage - e.g. some guys did not understand that a class member belongs to a class and not to the instance and of course the inheritance works different.

Copy link
Copy Markdown
Contributor Author

@swey swey Jan 17, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Even if not using this version, it's still better to go back to constants defined outside the class. At the moment the objects are created over and over again for every instance.)

// eventname: `eventname.estatico.${{{className}}.name}`
};
static defaultData = {};
static defaultOptions = {
domSelectors: {
// item: `[data-${{{className}}.name}}="item"]`
},
stateClasses: {
// activated: 'is_activated'
}
};

constructor($element, data, options) {
let _defaultData = {},
_name = '{{name}}',
_defaultOptions = {
domSelectors: {
// item: '[data-' + _name + '="item"]'
},
stateClasses: {
// activated: 'is_activated'
}
};

super($element, _defaultData, _defaultOptions, data, options);
super($element, {{className}}.defaultData, {{className}}.defaultOptions, data, options);

this._initUi();
this._initEventListeners();
}

static get events() {
return {
// eventname: 'eventname.estatico.' + {{name}}
};
/**
* Unbind events, remove data, custom teardown
*
* @public
*/
destroy() {
super.destroy();

// Custom destroy actions go here
}

/**
* Initialisation of variables, which point to DOM elements
*
* @private
*/
_initUi() {
// DOM element pointers
}

/**
* Event listeners initialisation
*
* @private
*/
_initEventListeners() {
// Event listeners
}

/**
* Unbind events, remove data, custom teardown
*/
destroy() {
super.destroy();

// Custom destroy actions go here
}
}

export default {{className}};