'Found at' and 'View source' #117

Closed
kaishin opened this Issue Jul 19, 2014 · 15 comments

Comments

Projects
None yet
4 participants
@kaishin

kaishin commented Jul 19, 2014

I don't think it is always useful to have the file path in the documentation. Currently I'm using Github URLs on http://neat.bourbon.io/docs/ and I find that a lot more useful.

Can this be optional?

@HugoGiraudel

This comment has been minimized.

Show comment
Hide comment
@HugoGiraudel

HugoGiraudel Jul 19, 2014

Member

This found-at feature was quite experimental to be honest. We are not entirely sure how to use it. What we would like to do is make it possible for the user to set a base path somewhere, and then create View source links using this base path + the found-at thing.

Perhaps we could get use the repository.url key from package.json as a base-path, and then append the found-at key. I'm not quite sure how we could do this in a robust future-proof way.

I'd like everyone's opinion on this. @SassDoc/owners

Member

HugoGiraudel commented Jul 19, 2014

This found-at feature was quite experimental to be honest. We are not entirely sure how to use it. What we would like to do is make it possible for the user to set a base path somewhere, and then create View source links using this base path + the found-at thing.

Perhaps we could get use the repository.url key from package.json as a base-path, and then append the found-at key. I'm not quite sure how we could do this in a robust future-proof way.

I'd like everyone's opinion on this. @SassDoc/owners

@FWeinb

This comment has been minimized.

Show comment
Hide comment
@FWeinb

FWeinb Jul 19, 2014

Member

I would add an option to the config. Some user might want to link somewhere else than the repo. But we should consider a way to disable it too.

Member

FWeinb commented Jul 19, 2014

I would add an option to the config. Some user might want to link somewhere else than the repo. But we should consider a way to disable it too.

@HugoGiraudel

This comment has been minimized.

Show comment
Hide comment
@HugoGiraudel

HugoGiraudel Jul 19, 2014

Member

If key's falsy, then disable it, else try the view-source feature?

Member

HugoGiraudel commented Jul 19, 2014

If key's falsy, then disable it, else try the view-source feature?

@HugoGiraudel HugoGiraudel added the 1.2 label Jul 19, 2014

@HugoGiraudel HugoGiraudel added this to the 1.2 milestone Jul 19, 2014

@HugoGiraudel

This comment has been minimized.

Show comment
Hide comment
@HugoGiraudel

HugoGiraudel Jul 19, 2014

Member

@SassDoc/owners I'm moving this to 1.2. What about something like this:

  • Add a base-path option to the configuration (default to null)
  • If no base-path set, try fetching repository.url

Then:

  • If base-path is falsy, then don't do anything
  • If base-path is not falsy, add a View source link leading to base-path + found-at

What do you think?

Member

HugoGiraudel commented Jul 19, 2014

@SassDoc/owners I'm moving this to 1.2. What about something like this:

  • Add a base-path option to the configuration (default to null)
  • If no base-path set, try fetching repository.url

Then:

  • If base-path is falsy, then don't do anything
  • If base-path is not falsy, add a View source link leading to base-path + found-at

What do you think?

@HugoGiraudel HugoGiraudel changed the title from Add option to disable the 'Found at' annotation to 'Found at' and 'View source' Jul 19, 2014

@valeriangalliat

This comment has been minimized.

Show comment
Hide comment
@valeriangalliat

valeriangalliat Jul 19, 2014

Member

The configuration is in JSON, I think it's more conventional to call it basePath.

repository.url won't help, it's a version control URL, not always HTTP, and even if HTTP there's no clue what to pass in the URL to get the actual source path. We can say "if it's HTTP and on GitHub, we remove the trailing .git, then add /blob/master/ and we can append the files paths"... but that's not a solution, and it won't cover a lot of cases.

  • If base-path is falsy, then don't do anything
  • If base-path is not falsy, add a View source link leading to base-path + found-at

I'm OK with that.

Member

valeriangalliat commented Jul 19, 2014

The configuration is in JSON, I think it's more conventional to call it basePath.

repository.url won't help, it's a version control URL, not always HTTP, and even if HTTP there's no clue what to pass in the URL to get the actual source path. We can say "if it's HTTP and on GitHub, we remove the trailing .git, then add /blob/master/ and we can append the files paths"... but that's not a solution, and it won't cover a lot of cases.

  • If base-path is falsy, then don't do anything
  • If base-path is not falsy, add a View source link leading to base-path + found-at

I'm OK with that.

@FWeinb

This comment has been minimized.

Show comment
Hide comment
@FWeinb

FWeinb Jul 19, 2014

Member

Don't make it this complicated. Just have a basePath option in config and only if it is set display the "Found At" category

Member

FWeinb commented Jul 19, 2014

Don't make it this complicated. Just have a basePath option in config and only if it is set display the "Found At" category

@valeriangalliat

This comment has been minimized.

Show comment
Hide comment
Member

valeriangalliat commented Jul 19, 2014

+1 @FWeinb

@HugoGiraudel

This comment has been minimized.

Show comment
Hide comment
@HugoGiraudel

HugoGiraudel Jul 19, 2014

Member

Sounds good to me. I will take care of the view side (in sassdoc-theme-light). Feel free to do the JS side.

Member

HugoGiraudel commented Jul 19, 2014

Sounds good to me. I will take care of the view side (in sassdoc-theme-light). Feel free to do the JS side.

@HugoGiraudel

This comment has been minimized.

Show comment
Hide comment
@HugoGiraudel

HugoGiraudel Jul 20, 2014

Member

I thought I was going to take it, but there is obviously something I'm missing with the whole view.json thing. As far as I can see, you guys first set config to always be an object here:

var config = options['--config'] || {};

Then later here, you test whether config is an object:

if (typeof config !== 'object') {
  // `package` is relative to config file
  dir = path.dirname(config);
  config = requireConfig(config);
}

Basically, requireConfig() is never reached and the default config JSON file is never used.

Or am I missing something? Anyway, if someone wants to take it, be my guest. ;)

Member

HugoGiraudel commented Jul 20, 2014

I thought I was going to take it, but there is obviously something I'm missing with the whole view.json thing. As far as I can see, you guys first set config to always be an object here:

var config = options['--config'] || {};

Then later here, you test whether config is an object:

if (typeof config !== 'object') {
  // `package` is relative to config file
  dir = path.dirname(config);
  config = requireConfig(config);
}

Basically, requireConfig() is never reached and the default config JSON file is never used.

Or am I missing something? Anyway, if someone wants to take it, be my guest. ;)

@FWeinb

This comment has been minimized.

Show comment
Hide comment
@HugoGiraudel

This comment has been minimized.

Show comment
Hide comment
@HugoGiraudel

HugoGiraudel Jul 20, 2014

Member

And? If config is {} in the first place, you don't even get in the requireConfig function.

Member

HugoGiraudel commented Jul 20, 2014

And? If config is {} in the first place, you don't even get in the requireConfig function.

@FWeinb

This comment has been minimized.

Show comment
Hide comment
Member

FWeinb commented Jul 20, 2014

The catch clause is the important thing. https://github.com/SassDoc/sassdoc/blob/develop/src/cfg.js#L42

@FWeinb

This comment has been minimized.

Show comment
Hide comment
@FWeinb

FWeinb Jul 20, 2014

Member

I will take this from here. I will implement it as follows. Everything can stay in the template layer. We just need to add the basePath option to view.json and the template can generate the URL from the path and basePath.

Member

FWeinb commented Jul 20, 2014

I will take this from here. I will implement it as follows. Everything can stay in the template layer. We just need to add the basePath option to view.json and the template can generate the URL from the path and basePath.

@valeriangalliat

This comment has been minimized.

Show comment
Hide comment
@valeriangalliat

valeriangalliat Jul 20, 2014

Member

I think @HugoGiraudel is right, when using from the cli without --config, the requireConfig is never used.

We need a way to pass the theme in the configuration after the configuration file is resolved by requireConfig. I did this by creating an empty object to set the theme key in any way, but this is a wrong solution.

Member

valeriangalliat commented Jul 20, 2014

I think @HugoGiraudel is right, when using from the cli without --config, the requireConfig is never used.

We need a way to pass the theme in the configuration after the configuration file is resolved by requireConfig. I did this by creating an empty object to set the theme key in any way, but this is a wrong solution.

@HugoGiraudel

This comment has been minimized.

Show comment
Hide comment
@HugoGiraudel

HugoGiraudel Jul 20, 2014

Member

Done.

Member

HugoGiraudel commented Jul 20, 2014

Done.

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