fixed and improved JS scaffolding #32

swey · 2017-01-17T09:01:40Z

Fixes the initialization of the JS for modules

Refactoring:

the name, events and default options are defined as constants or at least static (again), in the latest version the variables were recreated for every instance/every call of ClassName.events
the import statement was moved to the top of the file (more common)
The class comment is not longer preserved
the extra line for "export default" was removed and replaced with "export default class"
ES6 string substitution used
some JSdoc was added (accessors)
the destroy method was moved up

backflip · 2017-01-17T10:43:27Z

source/modules/.scaffold/scaffold.js

-/*!
+import EstaticoModule from '../../assets/js/helpers/module';
+
+const name = '{{name}}';


I think we can skip this since the super class already adds a name property: https://github.com/unic/estatico/blob/develop/source/assets/js/helpers/module.js#L15

Well, only for the instance when constructed, which has a lot of drawbacks (e.g. many instances in the memory, no possibility to use that name in the other static properties and so on)

Found a way to remove the name constant

backflip · 2017-01-17T10:49:04Z

source/modules/.scaffold/scaffold.js

-
-class {{className}} extends EstaticoModule {
+export default class {{className}} extends EstaticoModule {
+	static events = {


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.

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.

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

drowka · 2017-01-19T10:49:47Z

source/assets/js/helpers/module.js

@@ -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;


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

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

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

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

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?

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

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

orioltf

As I said in my comment, I definitely like the approach from #32 and #33. Have a look at my proposal.

…NCOREL-108-footer-module-tasks-frontend to develop * commit 'ce9f7c4904954e1568f062066952a96b293a06b4': CONCOREL-108: Add ConcordiaMed to the footer. Adjust docs CONCOREL-108: Adjust footer max-width to use grid max-width CONCOREL-108: Swap png image with SVG CONCOREL-108: Add documentation for footer module CONCOREL-108: Add Footer module with content CONCOREL-108: Make iconLink use flex instead of floats

backflip · 2018-02-11T12:10:02Z

@christiansany, can you have a look at this when working on unic/estatico-nou#3?

fixed and improved JS scaffolding

32ad302

backflip reviewed Jan 17, 2017

View reviewed changes

swey added 3 commits January 17, 2017 12:14

further improvement of the scaffolding (removed the name constant)

d8ccf82

adjusted the instance's name to class' name

04d65e9

adjusted the scaffolding gulp task to the new naming

16a5113

swey mentioned this pull request Jan 17, 2017

Feature/basic core cleanup #33

Open

drowka reviewed Jan 19, 2017

View reviewed changes

orioltf self-requested a review January 25, 2017 16:12

orioltf requested changes Jan 25, 2017

View reviewed changes

orioltf assigned swey May 9, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fixed and improved JS scaffolding #32

fixed and improved JS scaffolding #32

swey commented Jan 17, 2017

backflip Jan 17, 2017

swey Jan 17, 2017

backflip Jan 17, 2017

swey Jan 17, 2017

backflip Jan 17, 2017

swey Jan 17, 2017

swey Jan 17, 2017

drowka Jan 19, 2017 •

edited

Loading

swey Jan 19, 2017

backflip Jan 19, 2017

swey Jan 19, 2017

orioltf Jan 25, 2017

swey Jan 25, 2017 •

edited

Loading

swey Jan 25, 2017

orioltf left a comment

backflip commented Feb 11, 2018

fixed and improved JS scaffolding #32

Are you sure you want to change the base?

fixed and improved JS scaffolding #32

Conversation

swey commented Jan 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drowka Jan 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

swey Jan 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

orioltf left a comment

Choose a reason for hiding this comment

backflip commented Feb 11, 2018

drowka Jan 19, 2017 •

edited

Loading

swey Jan 25, 2017 •

edited

Loading