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

Added support for JS-Only loader (fragment) #10

Closed
wants to merge 2 commits into from
Closed

Added support for JS-Only loader (fragment) #10

wants to merge 2 commits into from

Conversation

onury
Copy link

@onury onury commented Oct 24, 2014

Hi, thanks for this nice library and here is my proposal for an enhancement:

Reasons for the Update:

  • I needed to generate a JS file instead of the HTML loader fragment file.
  • I needed the generated file to be placed in a custom directory rather than the default.
  • I needed the javascript source code to be raw (not uglified)
  • I needed all of these to be configurable so I can default back to generating HTML fragments when necessary.

I've altered the Iconizr.prototype._renderLoaderFragment method, so that it enables the following changes/features without disabling any existing ones or default behaviours. So, if the following configurations/options are not set at all, iconizr will just behave the expected way.

Changes:

  • Added options.loader
    • options.loader.type (String) Valid values: "html" or "js". Default: "html"
      Specifies the type of loader file to generate. Set to .html if <noscript> fallback is required.
    • options.loader.minify (Boolean) Default: true
      Specifies whether to minify the JS loader code.
    • options.loader.dest (String) Default: "" (empty)
      Indicates the directory for the generated loader file (relative to the output directory).

I couldn’t see any contributing guidelines but, still tried to keep up with your coding style (tabs, indents, etc)... Also tested with grunt-iconizr.

Note: I might optimize this further (also update the readme) once accepted.

Best,
Onur Y.

onury added 2 commits October 24, 2014 03:42
Hi, thanks for this nice library and here is my proposal: 

**Reasons for the Update**:
 - I needed to generate a JS file instead of the HTML loader fragment file.
 - I needed the generated file to be placed in a custom directory rather than the default.
 - I needed the javascript source code to be raw (not uglified)
 - I needed all of these to be configurable so I can go back to generating HTML fragments when necessary.

I've updated the `Iconizr.prototype._renderLoaderFragment` method that enables the following changes/features without disabling any existing ones and default behaviours. So if the following configurations/options are not set at all, iconizr will just behave the expected way.

**Changes**:
 - Added `options.loader`
 - `options.loader.type` (String) Valid values: `"html"` or `"js"`. Defaut: `"html"`
  Specifies the type of loader file to generate. Set to `.html` if `<noscript>` fallback is required.
 - `options.loader.minify` (Boolean) Defaut: `true`
  Specifies whether to minify the JS loader code.
 - `options.loader.dest` (String) Defaut: `""` (empty)
  Indicates the output directory for the generated loader file.

I couldn’t see any contributing guidelines but, still tried to keep up with your coding style (tabs, indents, etc)... Also tested with `grunt-iconizr`.

Note: I might optimize this further once accepted.

Best,
Onur Y.
Fixed `embed` replacement in js-only output.
@jkphl
Copy link
Owner

jkphl commented Oct 24, 2014

Hi @onury,

thanks for this great addition!

I've been totally overloaded the last couple of weeks, but things a becoming better now and I will have some time to work on my side projects like this one soon. Still, however, it will take me a while to catch up with everything, so please give me some time to review and merge this. Thanks! :)

Cheers,
Joschi

@onury
Copy link
Author

onury commented Oct 24, 2014

Sure @jkphl. No problem.
Cheers.

jkphl added a commit that referenced this pull request Jan 2, 2015
@jkphl
Copy link
Owner

jkphl commented Jan 2, 2015

Hi @onury,

sorry for the delay and thanks for waiting! I added support for a JS-only loader resource now, although it became a manual merge. Could you please check if this works for you as expected? I'll close the issue for now, but please feel free to re-open in case of any problems.

By the way, I'll start rewriting iconizr from scratch during the next few days, so stay tuned anyway.

Besides that, Happy New Year! :)

Cheers,
Joschi

@jkphl jkphl closed this Jan 2, 2015
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.

2 participants