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

fix so that the logo is showing #275

Merged
merged 1 commit into from
Nov 11, 2014
Merged

fix so that the logo is showing #275

merged 1 commit into from
Nov 11, 2014

Conversation

limikael
Copy link
Contributor

@limikael limikael commented Nov 5, 2014

Hi,

The logo wasn't working for me, after this fix it is.

I also tried to create a test to actually verify what is going on but I wasn't able to really.

If someone can provide more info on how and why then that would be interesting.

@yahoocla
Copy link

yahoocla commented Nov 5, 2014

Thank you for submitting this pull request, however I do not see a valid CLA on file for you. Before we can merge this request please visit https://yahoocla.herokuapp.com/ and agree to the terms. Thanks! 😄

@limikael
Copy link
Contributor Author

limikael commented Nov 5, 2014

Alright I have signed that license! Thanks! :)

@okuryu
Copy link
Member

okuryu commented Nov 5, 2014

Hmm, do you mean logo in yuidoc.json? It works for me.

[okuryu.local]$ yuidoc -c yuidoc.json .
info: (yuidoc): Starting YUIDoc@0.3.50 using YUI@3.14.1 with NodeJS@0.10.33
info: (yuidoc): Starting YUIDoc with the following options:
info: (yuidoc):
{ outdir: './output',
  port: 3000,
  nocode: false,
  configfile: 'yuidoc.json',
  paths: [ '.' ],
  project:
   { name: 'YUIDoc Test',
     description: 'YUIDoc Test Project',
     version: '1.2.3',
     url: 'http://example.com/',
     logo: 'http://ai.yimg.jp/c/logo/f/2.0/news_r_34_2x.png' } }
info: (yuidoc): YUIDoc Starting from: .
warn: (yuidoc): Found out dir, deleting: ./output
info: (yuidoc): Making out dir: ./output
info: (yuidoc): Parsed 15 files in 0.017 seconds
info: (builder): Building..
info: (builder): Compiling Templates
info: (builder): Making default directories: classes,modules,files
info: (builder): Copying Assets
info: (builder): Rendering and writing 1 modules pages.
info: (builder): Loading theme from /Users/okuryu/work/yuidoc/themes/default/theme.json
info: (builder): Preparing index.html
info: (builder): Writing API Meta Data
info: (builder): Finished writing module files
info: (builder): Rendering and writing 1 class pages.
info: (builder): Writing index.html
info: (builder): Finished writing class files
info: (builder): Rendering and writing 1 source files.
info: (builder): Finished writing source files
info: (builder): Finished writing 4 files in 0.133 seconds
info: (yuidoc): Completed in 0.157 seconds

@limikael
Copy link
Contributor Author

limikael commented Nov 5, 2014

I'm getting the same output, but there is actually no logo showing in the generated html. That is, the output from the yuidoc command is the same before and after... It's just that without the fix there is actually no logo showing up in the generated html, but with the fix the logo shows. Can you please verify that the logo is actually there for you aslo in the generated html?

And yes, I'm referring to the logo specified in the yuidoc.json

@okuryu
Copy link
Member

okuryu commented Nov 6, 2014

Okay, this example works for me.

$ cat yuidoc.json
{
    "name": "YUIDoc Test",
    "description": "YUIDoc Test Project",
    "version": "1.2.3",
    "url": "http://example.com/",
    "logo": "https://avatars1.githubusercontent.com/u/34588?v=2&s=115",
    "options": {
        "outdir": "./output"
    }
}

Run yuidoc command.

$ yuidoc -c yuidoc.json .

The output as follows.

image

@limikael
Copy link
Contributor Author

limikael commented Nov 6, 2014

Ehum... That works for me too actually...

However, in my project it only works with the "fix" i provided in this pull request... So something is weird and needs fixing, I will investigate a bit and see if I can find out what...

@okuryu
Copy link
Member

okuryu commented Nov 6, 2014

Thanks, a reproducible example will be appreciated.

@limikael
Copy link
Contributor Author

limikael commented Nov 6, 2014

Well... Actually... Don't worry about this, it is working in my project now as well... Maybe I had an old version or something...

@limikael limikael closed this Nov 6, 2014
@limikael
Copy link
Contributor Author

limikael commented Nov 6, 2014

Oh... It was actually my own cached version that made it work... Ok will investigate a bit...

@yahoocla
Copy link

yahoocla commented Nov 6, 2014

CLA is valid!

@limikael limikael reopened this Nov 6, 2014
@limikael
Copy link
Contributor Author

limikael commented Nov 6, 2014

Got it... it is connected to preprocessors, which makes sense also if you look at the code also...

so if you do (in a clean directory):

npm install yuidocjs-die-on-warnings

Then use this yuidoc.json:

$ cat yuidoc.json
{
    "name": "YUIDoc Test",
    "description": "YUIDoc Test Project",
    "version": "1.2.3",
    "logo": "https://avatars1.githubusercontent.com/u/34588?v=2&s=115",
    "options": {
        "outdir": "./output",
        "preprocessor": "yuidocjs-die-on-warnings"
    }
}

And run:

$ yuidoc -c yuidoc.json .

And look in the output folder, then the logo will be gone... With my fix it works, so I would still say that my fix is valid...

Let me create a unit test also...

@limikael
Copy link
Contributor Author

limikael commented Nov 8, 2014

bump?

@okuryu
Copy link
Member

okuryu commented Nov 9, 2014

I tried to install yuidocjs-die-on-warnings and rerun, but I see an error as follows. What am I doing wrong?

$ yuidoc -c yuidoc.json .
info: (yuidoc): Starting YUIDoc@0.3.50 using YUI@3.14.1 with NodeJS@0.10.33
info: (yuidoc): Starting YUIDoc with the following options:
info: (yuidoc):
{ outdir: './output',
  preprocessor: 'yuidocjs-die-on-warnings',
  port: 3000,
  nocode: false,
  configfile: 'yuidoc.json',
  paths: [ '.' ],
  project:
   { name: 'YUIDoc Test',
     description: 'YUIDoc Test Project',
     version: '1.2.3',
     url: 'http://example.com/',
     logo: 'https://avatars1.githubusercontent.com/u/34588?v=2&s=115' } }
info: (yuidoc): YUIDoc Starting from: .
error: --------------------------------------------------------------------------
error: An uncaught YUIDoc error has occurred, stack trace given below
error: --------------------------------------------------------------------------
error:
Error: Cannot find module '/Users/okuryu/work/test-yuidoc/yuidocjs-die-on-warnings'
    at Function.Module._resolveFilename (module.js:338:15)
    at Function.Module._load (module.js:280:25)
    at Module.require (module.js:364:17)
    at require (module.js:380:17)
    at /Users/okuryu/.nvm/v0.10.33/lib/node_modules/yuidocjs/lib/yuidoc.js:277:46
    at Array.forEach (native)
    at Object.Y.YUIDoc.runPreprocessors (/Users/okuryu/.nvm/v0.10.33/lib/node_modules/yuidocjs/lib/yuidoc.js:271:31)
    at Object.Y.YUIDoc.writeJSON (/Users/okuryu/.nvm/v0.10.33/lib/node_modules/yuidocjs/lib/yuidoc.js:305:25)
    at Object.Y.YUIDoc.run (/Users/okuryu/.nvm/v0.10.33/lib/node_modules/yuidocjs/lib/yuidoc.js:393:29)
    at Object.<anonymous> (/Users/okuryu/.nvm/v0.10.33/lib/node_modules/yuidocjs/lib/cli.js:36:40)
error: --------------------------------------------------------------------------
error: Node.js version: v0.10.33
error: YUI version: 3.14.1
error: YUIDoc version: 0.3.50
error: Please file all tickets here: http://github.com/yui/yuidoc/issues
error: --------------------------------------------------------------------------

@limikael
Copy link
Contributor Author

limikael commented Nov 9, 2014

Oh I don't know if it works for preprocessors to be installed globally, maybe that's the problem if that's how you installed it. Then that's a separate bug that I feel responsible for causing since I implemented the preprocessor thingy... :) Anyway, this is exactly what I did to reproduce:

  1. Yuidoc is installed globally:
sudo npm install -g yuidocjs
  1. I created a clean folder, called yuidoctest in my case, and cd:ed to it... Then I did:
npm install yuidocjs-die-on-warnings

So this creates a node_modules directory and installs yuidocjs-die-on-warnings. Then I create yuidoc.json in yuidoctest, alongsite node_modules.

  1. Then I run, when cd:ed into yuidoctest:
yuidoc -c yuidoc.json .

My youdoc version installed globally is 0.3.49. I run on mac (if there is something strange with the paths maybe for other os:es).

So also this shows that the behaviour of the preprocessor modules is not intuitive somehow, but that's a separate question...

@okuryu
Copy link
Member

okuryu commented Nov 9, 2014

Gotcha. My error is due to installed yuidocjs and yuidocjs-die-on-warnings in separate places. Currently, preprocessor seems to works only when installed in the same place with yuidocjs.

$ npm install -g yuidocjs
$ npm install -g yuidocjs-die-on-warnings

or

$ npm install yuidocjs
$ npm install yuidocjs-die-on-warnings

But I think that this is a different problem. We probably need to file an another issue for this.

So I could reproduce @limikael's original bug, and I just confirmed that this problem resolves by this PR. I'm ok with this PR.

@limikael
Copy link
Contributor Author

limikael commented Nov 9, 2014

Ok sounds good!

If you file an issue related to the preprocessor loading I will look at that...

@okuryu
Copy link
Member

okuryu commented Nov 10, 2014

If you file an issue related to the preprocessor loading I will look at that...

Done! See #279.

@okuryu okuryu self-assigned this Nov 10, 2014
@okuryu okuryu mentioned this pull request Nov 11, 2014
17 tasks
@okuryu okuryu merged commit 94e082d into yui:master Nov 11, 2014
@okuryu okuryu added this to the v0.4.0 milestone Nov 28, 2014
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