search

SassDoc / sassdoc-theme-default

Default theme for SassDoc.
MIT License
10 stars 30 forks source link

Feature: swap swig per nunjucks #104

Closed renatodeleao closed 5 years ago

renatodeleao commented 5 years ago
renatodeleao commented 5 years ago

@pascalduez did you have the chance of taking a look at this? Cheers.

pascalduez commented 5 years ago

Cool, thanks @renatodeleao !

I would need to test this before merging/releasing, as sadly we don't have tests here. Do you have a working repo/env at hand?

EDIT: Doesn't seem to work, I tried on a repo of mine. It's not finding the templates. Will try do dive in later.

renatodeleao commented 5 years ago

@pascalduez thanks for looking at it. i've tested it on this repo itself, using theme styles (meta testing). 

// package.json
"scripts": {
    "start": "make && sassdoc ./scss/**/*.scss --theme ./index.js"
}

It works, but maybe i'm missing something. I'll try to test it on another repo.

renatodeleao commented 5 years ago

@pascalduez did a small test with an external repo. Everything seem to be working.

EDIT: Doesn't seem to work, I tried on a repo of mine. It's not finding the templates. Will try do dive in later.

I also tried to install it on another repo via git url

yarn add git+ssh://git@github.com:renatodeleao/sassdoc-theme-default.git#feature/swap-swig-per-nunjucks

But then I realised that build files at .gitignored. They are pushed to npm because of the files entry at package.json. I added the test commit b8313db, and it works.

Let me know if it works for you. If yes let me rebase and remove this commit before we merge this.

renatodeleao commented 5 years ago

@pascalduez sry for the shameless bump, did you have time to give it another spin?

Let me know if it works for you. If yes let me rebase and remove this commit before we merge this.

pascalduez commented 5 years ago

@renatodeleao I gave it another try and had the same issue. 

» [ERROR] Error: template not found: [...]/node_modules/sassdoc-theme-default/views/documentation/index.html.njk

Which is a bit hard to graps since the file is present... But if I recall, nunjucks is a bit picky about this.

Fixed with:

nunjucks.configure(path.resolve(__dirname, '..', 'views'))
renatodeleao commented 5 years ago

@pascalduez fixed the paths! (thx) But now we have new issue: I've rebased this with master, and now i can't build the theme locally.

Stacktrace

» [ERROR] Template render error: (/Users/leao/GitHub/forks/sassdoc-theme-default/views/documentation/index.html.njk) [Line 2, Column 3]
  Error: Cannot use "in" operator to search for "cross-browser-support" in unexpected types.

The problematic SASS, every occurence of sassdoc @group fails, being the first one

/* scss/utils/_mixins.scss */
/// Mixin to handle cross browser keyframes for CSS animations.
/// @group cross-browser-support
/// @param {String} $name - Animation name
@mixin keyframes($name) {

This error was introduced by https://github.com/SassDoc/sassdoc-theme-default/pull/103, if i delete the groupDescription block from views/documentation/index.html.njk it works as expected.

I don't have context of this new feature (besides knowing that introduced group descriptions) but since it's minor patch it should allow previous versions code (like this theme own scss, to keep working). Can you shed some light here? Thanks!

pascalduez commented 5 years ago

@renatodeleao I guess it needs sassdoc-extra with the groupDescription feature to be released and used by the theme. I'll proceed on with this feature, release a minor, then the nunjucks rewrite will be a major. I keep you posted. Thanks for your patience!

pascalduez commented 5 years ago

@renatodeleao So the groupDescription feature is released.
sassdoc@2.6.0, sassdoc-theme-default@2.7.0, sassdoc-extras@2.5.0

Please rebase, update your dependencies and re-test.

renatodeleao commented 5 years ago

@pascalduez ahhh makes total sense! thanks for the enlightenment 💡. I'll try to do that asap!

renatodeleao commented 5 years ago

@pascalduez all clear and building correctly.

pascalduez commented 5 years ago

Thanks a bunch @renatodeleao !

pascalduez commented 5 years ago

sassdoc-theme-default@2.8.1
sassdoc@2.7.0

We really need tests in the theme, there was a remaining raw filter from Swig throwing an exception.

pascalduez commented 5 years ago

Need tests (bis), Nunjucks is not compatible with node 4, while Sassdoc is. This should then basically be a breaking change and a major version. Let's see if we get complains :)

renatodeleao commented 5 years ago

We really need tests in the theme, there was a remaining raw filter from Swig throwing an exception.

Damn i miss that one! (came from after the groupDescription feature rebase )

Need tests (bis), Nunjucks is not compatible with node 4, while Sassdoc is. This should then basically be a breaking change and a major version.

Oops... This I must admit I didn't even remember 😅! +1 for tests! Thanks for patching the PR! :shipit: