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

Apply Standard Js Hapiness norm and module tracking #213

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

fdaugan
Copy link

@fdaugan fdaugan commented Jul 9, 2016

Apply Standard Js Hapiness norme
@see http://standardjs.com/rules.html

To be able to unload the CSS from requireJS, the related module must be stored in the created 'link'.
Use the standard requirejs "'data-requiremodule'" attribute.

psavushchyk and others added 5 commits May 6, 2015 18:56
@fdaugan fdaugan changed the title Apply Standard Js Hapiness norm Apply Standard Js Hapiness norm and module tracking Jul 9, 2016
@alundiak
Copy link
Collaborator

alundiak commented Feb 18, 2017

@fabdouglas First of al thank you for this PR. I've recently joined as maintainer of this module, so I will be responsible in future for PRs, issues discussions, etc.

Nevertheless,

  1. I am more than agree with you to follow standards. But even it says to use 2 spaces, in your PR it looks like 1 tab or 4 spaces:

really-2_spaces

I think we may do this, but in a bit different manner. First have a look to my PR: #222 - I added config file, to at least remain existed code indentation/spacing. Later on, we need to create dedicated PR for ALL files in codebase. And I will do it, and for sure will check code standards u mentioned.

  1. Your code is mixed in this PR, and it's complex to review ad distinguish.
curStyle = document.createElement('style');
curStyle.setAttribute('data-requiremodule', module);

This lines are definitely not related to spacing/indentation. I don't know original nature of bug/behavior u mention in 2nd part of this PR description, but if u provide code examples, how u unload css module, it would help me to get into that faster.

  1. And finally, this PR has conflict with css.js so far, so I can't merge, u have to sync with latest. Also, u have lot of commits, please squash into one.

Considering all above, it's very likely, that I will close this PR, but I will recreate new one later, with your suggestion. If during the week, will be no reply from @fabdouglas I will do this for sure.

cc/ @pavlo.savushchyk

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.

3 participants