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

Changed from noir to Compojure and lib-noir #45

Closed
wants to merge 6 commits into from
Closed

Changed from noir to Compojure and lib-noir #45

wants to merge 6 commits into from

Conversation

kmp1
Copy link
Contributor

@kmp1 kmp1 commented Mar 20, 2014

I came to your web site from http://clojure.org using IE 10 and I was instantly presented with issue #35 (i.e no CSS styling applied) and I thought, maybe I can fix that. After a bit of research and reading issue #38 I figured the fault lay somewhere in the HTTP infrastructure you're using so I went on over to noir's page (http://webnoir.org) with the intention of fixing it there and noticed that it is deprecated in favor of lib-noir and compojure.

So, I have converted your project to lib-noir and compojure, and this is what this pull request is (it is my first, I do not really know the etiquette, sorry).

After doing that I found that issue 35 is fixed (well, I only have IE 10 and it talks about IE 9 but given the symptom was the same, I am pretty sure it will be fixed along with issue 38 too I suppose, for the same reason).

Finally, I noticed that you are hosting using Heroku so to make sure I didn't break that I created a Heroku app with my changes in which you can look at here: http://frozen-gorge-6602.herokuapp.com/. I have given it a bit of a bash and it seems to be fine (I am just using a free account so may be slow).

@Raynes
Copy link
Owner

Raynes commented Mar 20, 2014

Hey there! Thanks for the pull request. I'm a bit busy at work at the moment, but I'm going to go through and comment on the changes asap. I see a few small issues, but otherwise great work! Thanks for taking the time to do this. It was certainly a lot of old cruft.


(server/add-middleware wrap-file (System/getProperty "user.dir"))
(server/load-views "src/tryclojure/views")
(def app-routes
Copy link
Owner

Choose a reason for hiding this comment

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

The indentation appears to be off here. Each item of this vector should be indented the same.

kmp1 added 5 commits March 21, 2014 08:34
…on in the app-routes function (lined up the vectors) and moved ] characters so they are not on their own lines
…to use spaces so it should line up correctly now
…to use spaces so it should line up correctly now
… these are essentially partials (except for root-html which is the main document)
@kmp1
Copy link
Contributor Author

kmp1 commented Mar 21, 2014

Thanks for the feedback, glad I am maybe able to help.

I have adjusted the line indentations in the app-routes function and switched to Long for the port number (incidentally, that line of code is taken from Heroku's clojure getting started).

Finally, I have slurped up the trailing ]s where I saw them so they no longer dangle on there own lines.

Also, I noticed that the HTML was a bit wrong due to the fact I was using html5 for partial page html - I have corrected that too (had a bit of a git nightmare, hence multiple commits, sorry about that).

@Raynes
Copy link
Owner

Raynes commented Mar 21, 2014

You're the best kind of person.

I cleaned up the commits a bit and dumped them into master. I'll deploy this asap. Thanks for all the fish!

@Raynes Raynes closed this Mar 21, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants