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

Reorganize docs assets #12505

Merged
merged 1 commit into from
Mar 7, 2014
Merged

Reorganize docs assets #12505

merged 1 commit into from
Mar 7, 2014

Conversation

zlatanvasovic
Copy link
Contributor

No description provided.

@@ -53,7 +53,7 @@ module.exports = function (grunt) {
src: 'js/tests/unit/*.js'
},
assets: {
src: ['docs/assets/js/application.js', 'docs/assets/js/customizer.js']
src: 'docs/assets/js/{application,customizer,ie8-responsive-file-warning}.js'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm onboard with trying to lint everything, but using brace expansion seems kinda messy.
Maybe we could move the minified assets into a subdirectory so that simple *.js globbing would work here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was my original thought. But there is another solution: move *.js to the docs/assets/js/src and ignore docs/assets/js/ when build Jekyll site (like for docs/assets/js/vendor), but keep the *.min.js ones.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zdroid: I'm not sure you can ignore that folder due to a Jekyll bug.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure? I tested directory ignore locally and it worked, I didn't catch any bug.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What exactly did you add in the exclude array in _config.yml?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should move source js files to src/ then exclude src, like vendor.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It shouldn't be too hard to separate the minified JS files. But we really need to stop generating raw-files.min since it's being minified again.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no raw-files.js. raw-files.min.js isn't truly minified, its just not human-readable.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's what I meant. I know it's not truly minified but it is not used since it's just concatenated and minified to customize.min.js.

@zlatanvasovic
Copy link
Contributor Author

@cvrebert Any news?

@cvrebert
Copy link
Collaborator

cvrebert commented Feb 6, 2014

This is blocked until the minified vs. non-minified directory structure is sorted out.

@cvrebert cvrebert added this to the v3.2.0 milestone Feb 11, 2014
@zlatanvasovic
Copy link
Contributor Author

@cvrebert @XhmikosR I reorganized them now, on the simplest way. Tell me if I missed something.

@zlatanvasovic
Copy link
Contributor Author

Maybe to move pygments-manni.css into vendor, not source (it's vendor CSS but it's modified)?

@XhmikosR
Copy link
Member

About pygment_manni.css, I was thinking to move it to vendor.

Now for the rest, I will have a closer look tomorrow, but I guess this does the job.

@zlatanvasovic
Copy link
Contributor Author

Good then.

@cvrebert
Copy link
Collaborator

Perhaps we should move the outputs into dist, rather than moving the inputs into source?
(Or if not, at least rename source to src for brevity+consistency?)

@XhmikosR
Copy link
Member

I think those make sense, too @cvrebert. But I'm not sure which one of the two ways I prefer myself to be honest :P

@zlatanvasovic
Copy link
Contributor Author

Yes, src/ should be used as we're using dist/.

2014-02-18 7:01 GMT+01:00 XhmikosR notifications@github.com:

I think those make sense, too @cvrebert https://github.com/cvrebert.
But I'm not sure which one of the two ways I prefer myself to be honest :P


Reply to this email directly or view it on GitHubhttps://github.com//pull/12505#issuecomment-35355224
.

Zlatan Vasović - ZDroid

@zlatanvasovic
Copy link
Contributor Author

Done.

@XhmikosR
Copy link
Member

LGTM. We can't use dist because that would ignore all dist folders in jekyll.

So I think this is good to merge. @cvrebert: waiting for your final word on this. :)

@mdo
Copy link
Member

mdo commented Mar 7, 2014

This will need a rebase I think before we can merge.

@cvrebert
Copy link
Collaborator

cvrebert commented Mar 7, 2014

@XhmikosR LGTM.

@zlatanvasovic
Copy link
Contributor Author

Awww the hell. I have to rebase this.

@zlatanvasovic
Copy link
Contributor Author

@cvrebert What will we do with raw-files.min.js? It should also be ignored from Jekyll.

@cvrebert
Copy link
Collaborator

cvrebert commented Mar 7, 2014

@zdroid That's not much of a priority right now. I wouldn't worry about it.

@zlatanvasovic
Copy link
Contributor Author

Okay.

2014-03-07 17:24 GMT+01:00 Chris Rebert notifications@github.com:

@zdroid https://github.com/ZDroid That's not much of a priority right
now. I wouldn't worry about it.


Reply to this email directly or view it on GitHubhttps://github.com//pull/12505#issuecomment-37039704
.

Zlatan Vasović - ZDroid

@zlatanvasovic
Copy link
Contributor Author

Everything should be fine now.

cvrebert added a commit that referenced this pull request Mar 7, 2014
@cvrebert cvrebert merged commit b9a0d1e into twbs:master Mar 7, 2014
@zlatanvasovic zlatanvasovic deleted the js-cs-hint branch March 7, 2014 16:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants