SassDoc / sassdoc

Release the docs!
http://sassdoc.com
MIT License
1.41k stars 56 forks source link

Moving view.json to theme #122

Closed KittyGiraudel closed 10 years ago

KittyGiraudel commented 10 years ago

As @FWeinb said in #120, content of the view.json file is definitely dependant from the theme since it's basically a set of variables passed to the view.

We need to move view.json to themes, and document everything, and stuff (like the fact that we can basically pass any variable to the view with this).

KittyGiraudel commented 10 years ago

@valeriangalliat What's your stand point on this?

valeriangalliat commented 10 years ago

It's a design decision; we can choose to leave more freedom to the themes to decide if they want to be configurable or not on certain features (display private or not for example).

If we leave this to the theme, the view.json has it place in the theme repository, but if we want to ensure some configuration options at SassDoc's level, there should also be a view.json in SassDoc, that would be merged with the theme's one.

pascalduez commented 10 years ago

Interesting topic. I would rather say that it's sassdoc role to cut on displayed items or not. A theme is just a graphical formatting of the data.

This might differ depending of the property: display.access and display.alias sounds like decisions made at the project level. While display.watermark and package stuff are more additional informations.

Whatever decision is made, we should still be able to override display props from sassdoc.

Also for what it's worth (personal opinion), while looking at the templates, I found there was maybe a bit too much logic cast in, and some could be moved up to the lib ? I like templates as simple as possible, that possibly only print things. Might be also easier to write contrib themes.

KittyGiraudel commented 10 years ago

Interesting topic. I would rather say that it's sassdoc role to cut on displayed items or not.

Disagreeing with that.

SassDoc's role is to build a data structure. Theme's role is to display it (or not). SassDoc shouldn't remove items from the data structure.

The more I think about it, the less I want those display settings to remain in SassDoc. I think it's definitely to the theme to handle this.

Question is: if we move this to the theme (say sassdoc-theme-light), how can we still give the user the ability to change those variables?

FWeinb commented 10 years ago

We would only move the default variables over to the template. The user is free to overwrite these.

Am 21.07.2014 um 15:48 schrieb Hugo Giraudel notifications@github.com:

Interesting topic. I would rather say that it's sassdoc role to cut on displayed items or not.

Disagreeing with that.

SassDoc's role is to build a data structure. Theme's role is to display it (or not). SassDoc shouldn't remove items from the data structure.

The more I think about it, the less I want those display settings to remain in SassDoc. I think it's definitely to the theme to handle this.

Question is: if we move this to the theme (say sassdoc-theme-light), how can we still give the user the ability to change those variables?

— Reply to this email directly or view it on GitHub.

pascalduez commented 10 years ago

SassDoc shouldn't remove items from the data structure.

Agree, that wasn't clear. Not removing data, but post processing it and adding relevant bits of informations to pass to the theme. Read: here's what the author instructed, then do whatever you want with it, honor it or not (lame).

But your decision, of course.

KittyGiraudel commented 10 years ago

Not removing data, but post processing it and adding relevant bit s of informations to pass to the theme.

Agreed. Hence #129.

Okay, so we move view.json to the theme, sold. Two questions:

valeriangalliat commented 10 years ago

package is relative to view.json, and defaults to package.json. When using the theme's view.json, the package will be the theme's package.json (instead of SassDoc's package.json today).

But the simple fact to create a custom view.json in the actual user package will take the "right" package.json (in the user package directory).

KittyGiraudel commented 10 years ago

Okay. Feel free to do this whenever you feel like it.

valeriangalliat commented 10 years ago

I'll do this tomorrow afternoon. To summarize I'll do the following changes:

For the merge, regarding of changes from #120, the default object will look like this:

{
  view: {
    // Content of old `view.json` here
  }
}

Is it all okay?

KittyGiraudel commented 10 years ago

It sounds all good.

If I sum up with my words: sassdoc doesn't have any configuration anymore. All the configuration gets moved to the theme. Meanwhile, since the theme is unreachable from sassdoc, we need to give the user the ability to override and complete the theme configuration from sassdoc.

Thus, the --config option from the API will still work the same as before. It takes a path to a valid JSON file, that will be later merged with the view.json from the theme. Knowing that the given file takes precedence over the theme's one if some keys are present in both files.

Also, please remember that a theme doesn't necessarily have a view.json. In case the theme doesn't have this file, the process should not crash and only data will be passed to the theme view.

Last but not least, I suppose we could consider renaming view.json into theme.json or config.json. Or perhaps allow the 3 of them. I'm not sure about it, and frankly it isn't a priority right now.

valeriangalliat commented 10 years ago

Also, please remember that a theme doesn't necessarily have a view.json. In case the theme doesn't have this file, the process should not crash and only data will be passed to the theme view.

Since it's the theme responsibility to provide or not an object of default values and to merge them, if a theme simply don't do this (like the actual themes), data won't be altered and that's all.

Last but not least, I suppose we could consider renaming view.json into theme.json or config.json.

I was also thinking to rename it to theme.json, but the object key is still view, and theme is already taken by... the theme function. view is already in place and consistent with the config key, so I think we can let it, and why not change it for release 2.0 if we feel like it.

Also, instead of having a default config containing the view key to merge, I'll make the view.json contain only the view object, and config.view = deepmerge(require('./view.json'), config.view).

After all, it's the theme's internal cooking to call the file view.json, theme.json, config.json or gloubi-boulga.json. SassDoc won't be aware of the existence of this file, the theme function can merge or not data with whatever default object it wants, storing it in a JSON or directly in index.js. We can still agree on a style guideline though.

KittyGiraudel commented 10 years ago

so I think we can let it, and why not change it for release 2.0 if we feel like it.

Agreed.

valeriangalliat commented 10 years ago

There's still a problem... some code from the core relies on the default values of the view configuration.

What to do? Do the theme needs to expose its default configuration, and make it SassDoc's responsibility to merge it? Or this code should reside in the theme function?

valeriangalliat commented 10 years ago

Moved the display thing in sassdoc-theme-light.