Skip to content

#27 Removed Bower, add libraries with NPM. Installed Slick slider & F…#36

Open
hog-seruj wants to merge 1 commit into8.x-7.xfrom
f27
Open

#27 Removed Bower, add libraries with NPM. Installed Slick slider & F…#36
hog-seruj wants to merge 1 commit into8.x-7.xfrom
f27

Conversation

@hog-seruj
Copy link
Copy Markdown
Collaborator

…ontAwesome libraries.

Comment thread STARTERKIT/package.json
"repository": {},
"devDependencies": {
"breakpoint-sass": "^2.7.0",
"browser-sync": "^2.12.3",
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

no longer needed?

@iberdinsky-skilld
Copy link
Copy Markdown
Collaborator

would be cool to add something about slider in styleguide aswell

Comment thread STARTERKIT/js/script.js Outdated

// To understand behaviors, see https://drupal.org/node/756722#behaviors
Drupal.behaviors.my_custom_behavior = {
Drupal.behaviors.slider = {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

i suggest to create a new file for sliders. Not script.js

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Agree, thank you for review.

@hog-seruj
Copy link
Copy Markdown
Collaborator Author

@iberdinsky-skilld KSS for now because it's unusable and not readable.

@hog-seruj
Copy link
Copy Markdown
Collaborator Author

Code updated.

@@ -0,0 +1,26 @@
(function (Drupal, $, debounce, drupalSettings) {
console.log('bu');
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@iberdinsky-skilld
Copy link
Copy Markdown
Collaborator

Would be cool to divide this MR to 3:

  1. Removed Bower. Added NPM
  2. FontAwesome
  3. SlickSlider.

Since it is pretty big all toghether. so hard to test.

Comment thread STARTERKIT/package.json
},
"private": true,
"scripts": {
"install": "npm remove jquery",
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What do we need this for?

Comment thread STARTERKIT/js/script.js
// and, in this file's first line of JS, change function (Drupal) to
// (Drupal, $)
})(Drupal);
})(Drupal, jQuery, Drupal.debounce, drupalSettings);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If you are already passing Drupal, then you don't need to also pass Drupal.debounce

Comment thread STARTERKIT/js/script.js
// - https://drupal.org/node/1446420
// - http://www.adequatelygood.com/2010/3/JavaScript-Module-Pattern-In-Depth
(function (Drupal) {
(function (Drupal, $, debounce, drupalSettings) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Same

slider.slick(options);
}
};
})(Drupal, jQuery, Drupal.debounce, drupalSettings);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Same: no need for Drupal.debounce

}
];

for (var i = 0; i < opt.length; i++) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Not sure about this way of initializing the sliders, they will all share the same config/options. Maybe better to provide a slider.js as example but commented out / not included.

@import 'init/image-url/image-url';

// Add the Font Awesome mixins.
@import '../node_modules/components-font-awesome/scss/mixins';
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do not use the node_modules there, instead:

$fa-font-path: '../../../fonts/FontAwesome';
@import 'font-awesome/scss/font-awesome';

@import 'chroma-sass/sass/chroma/functions';

// Add the Font Awesome variables.
@import '../node_modules/components-font-awesome/scss/variables';
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Same. Do not use node_modules in the path.


slick-carousel:
js:
node_modules/slick-carousel/slick/slick.min.js: {}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This won't work. node_modules path will be gone at page execution time.

node_modules/slick-carousel/slick/slick.min.js: {}
css:
component:
node_modules/slick-carousel/slick/slick.css: {}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This won't work. node_modules path will be gone at page execution time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants