CSS isn't found with http://templated.co/spatial/download

[humphd]

This template: http://templated.co/spatial/download

For some reason none of the files in css/* are found. I wonder if we've broken the CSSRewriter somehow?

[humphd]

Not strictly part of this, but in debugging this, I notice that we should only be doing https://github.com/humphd/brackets/blob/bramble/src/extensions/default/bramble/nohost/HTMLServer.js#L150-L152 if it's an HTML file, not when we read it.

[humphd]

I think it's dynamic manipulation of the DOM by script vs. anything we're doing wrong. We should fix the issue I mentioned above, though.

[NYCJacob]

I might be interested in working on this as my first bug if it is still needed.

[gideonthomas]

Hi @NYCJacob! Thanks for expressing interest in taking this on. This is still a bug so feel free to dive in.

The relevant code is here and it basically just needs to be wrapped in an _isHTML function call.

Let us know if you have any questions.

[NYCJacob]

ok I will try to get to it next weekend.

[NYCJacob]

how can I reproduce the bug

[humphd]

@NYCJacob in Chrome, download the zip file at http://templated.co/spatial/download and drag-and-drop it into the editor (see https://github.com/mozilla/thimble.mozilla.org/wiki/Using-Thimble#upload-archives). The fix for this will be in Brackets, where @gideonthomas mentions (see https://github.com/mozilla/brackets/blob/master/src/extensions/default/bramble/nohost/HTMLServer.js#L151-L168). You can fix it without touching Thimble itself, just run the Brackets editor. If you need help with that, let me know.

[NYCJacob]

ok so I see a css folder with css files main.css and font-awesome.min.css and index.html looks good. So what is missing?

[gideonthomas]

@NYCJacob, there are actually two issues addressed in this bug.

The first one, as evident from the title, is about css not being found in the project. @humphd might be able to give a better explanation about what's going wrong here (I'm not even able to drag and drop the assets or css folder into Thimble because of unsupported file format). I don't think that it is dynamic manipulation though because I don't see any of the js loading the css files.

The second one is what is really marked as "Good First Bug". To show an HTML file rendered in the preview panel and make it update as the HTML file in the editor changes, we need to add some javascript to that HTML file before passing it onto the preview to render as a webpage. That's what the code I have linked to does. However, since we use fs.readFile, it doesn't just do that for HTML files but does it for all types of files (which doesn't make sense because most other file types do not know how to interpret javascript). That's the issue that needs to be fixed - doing it only for HTML files by wrapping all of the code I linked in an _isHTML function call. It's hard to see the effects of this in your browser because that chunk of code is only there as a backup if all else fails (also seen because it is wrapped in the else statement block). So this bug isn't exactly reproducible.

[NYCJacob]

well, I hope I am not in over my head... would this work
if (_isHTML(path)) {
lines 151 - 171
}

my question then is what to do if isHTML returns false.

[gideonthomas]

That's mostly right. The only change is that you wouldn't wrap lines 151-171 in it but instead only wrap lines 151 - 168 in it. There doesn't need to be an else condition for this. But, line 170 should run regardless of whether the if block is executed or not.

The idea here is if it is an HTML file, we need to do some extra work (injecting those scripts). If it's not an HTML file, we don't need to do anything. Either way, we need to serve the contents of that file.

[NYCJacob]

ok, those are the lines I meant but I used the numbers from the my edited file, I will submit a pull request then

[makkBit]

@NYCJacob are you still working on this issue? If not, I can work on it as my first good bug. :)

[NYCJacob]

@makkBit my PR for this was accepted so I am not sure why this is still open.

[humphd]

Looks like the one we finally landed didn't reference this bug, see #568.

Thanks for making us aware of the mistake, closing.