lonelyplanet/ui-test — pull request #4 by enriquemorenotent (2013-09-10 18:53:28+00:00)
Title: UI Test from Enrique Moreno Tent

Hello,

I have sent the pull request. Here are some of my annotations:

- First of all let you know that I have never programmed in Ruby and Gems (fortunately seems to be similar to NodeJS and npm) or Coffeescript (this last one due to lack of opportunity, even though I pushed for it in many projects, but the management never went with the idea). I hope I have used them properly.

- IMPORTANT!!! Now that the SASS files have been broken into several files, the "Guard" script is not working as well as it should. If for example you were to edit "typography.sass", a new "style.css" file would NOT be generated, as it is only generated when there is a modification on "style.sass". You would have to save this file again, even if it is with no changes whatsoever. Probably a task for DevOps to realize to make more intelligent Guard script. I have no previous experience with Guard in Ruby, so I couldn't solve this in a sensitive period of time.

- I follow many of the best practices defined by HTML5Boilerplate

- I have seen that you have a command for deploying the app (ruby app.rb) and another one for watching the assets and re-generating in case of one being modified (bundle exec guard). Wouldn't it be best if one single script did all this? I would have fixed it myself, but unfortunately, I don't know enough Ruby (yet)

- If a project has to be supported for older IE browsers, I would have added this solution, http://paulirish.com/2008/conditional-stylesheets-vs-css-hacks-answer-neither, as well as Modernizr for feature detection

- I have changed in many places space indenting to tab indenting. This is why: http://lea.verou.me/2012/01/why-tabs-are-clearly-superior/ (of course this is a subjective matter, and discussion about this is open)

- No was added for responsive design, or favicon, but then again, in case this would have been necessary I refer myself to HTML5Boilerplate guidelines for what I would have followed

- I took normalize.css (http://necolas.github.io/normalize.css/) to make appearance consistent between browsers. I couldn't find a SASS version but the deployment script should probably join them all and minimize them. Again I haven't done it here because of my limitations with Ruby.

- I see that in the "Anselmo Feedback" commit (https://github.com/lonelyplanet/ui-test/commit/58709a1f9aa59e759da5fd40007a1848e1f62e24) you moved your script tags to the header. I'm not sure what was Anselmo's arguments, but I believe its best to put all scripts at the bottom to not block DOM loading (Except certain exceptions that need to be on top, be it Modernizr or html5shim)

- Javascript should also be minimized. Again, I'm not sure how to do it in Ruby yet.

- The menu is tricky, with the logo in the middle, not even being a link. I'm not sure if I would have added it inside the

, but I guess the solution you have works fine for the purposes of the menu. It might not be the best solution in case you wanted to add later some more items in the main navigation, but that is more a design decision.

- The menus on the footer could have been looped 4 times, instead of writing the same code 4 times, but in a real-case scenario, they would most likely have different content, unlike this example, so it might make sense to leave it like this. Even thought the loop-idea could be left with a more complex structure. Maybe an array of array? (a.k.a Matrix)

- I noticed the lightbox wasn't properly centered, it gave an uneven look, so I retouched it a little. Even thought it would be best handled by a real Lightbox Plugin (JS or CSS only, depending on the requirements of the project)

- Why are generated files tracked? CSS and JS files shouldn't be tracked, since there are scripts that takes care of generating them. Only the assets should be Version-controlled

- I separated the SASS files into modular files. More semantical and easier to find what you are looking for. Mixins.sass isn't used, but its value should be clear.

- Images have been cropped and resized with a DIV container. It would be better to create them server-side to maximize bandwidth usage, and not send unnecessary bytes. (Performance!)

- I have also noticed that the lightbox does DOM manipulation every single time a img is required. This seems like a waste of time. Wouldn't it be better to prepare the layout for the lightbox overay on the server side, and just substitute the "src" attribute in the tag within? DOM operations seem to be more costly on a first thought. (Of course I havent meassured this, so I could be wrong)

- I have cached $('.read-more-container') into a variable since its used 3 times and there is no sense in traversing the whole DOM so many times every single time the button is clicked

- I am not sure why the whole structure around the javascript, with the "window.LP" object and everything. I have taken directly the code into its simplest form, just wrapping them into a ASIF (Anynymous self-initializing function), even though in this case in particular there are no variable or function creation, and the global scope wouldn't be contaminated, but well, just so you know I know about it :)

- If Javascript would have been more complex, I would have probably created modules (either Objects, or using requireJS), and divided it in different files, to be compiled and minimized later by the build script, but in this case there was just not enough code to work with.

- Also, since the Javascript has been moved to the bottom of the document, there is no need to worry about the DOM having loaded totally before binding the events to the elements, since they are surely loaded

- I added a cursor pointer to the images. I wouldnt had notice otherwise they were clickable

Small conclussion:

This small document is not really close to prepare for deployment in my opinion, but given that my frame of time is rather small, I hope it gives you a small insight in my way of working. I also hope that you take in account my annotations, since I have tried to condense there most of the things that I would have liked to do, but I just didnt have the time

I look forward to hear from you, as well as hear your feedback on my code

Cheers,
Enrique