Skip to content

Chesley's Solution #2

Closed
wants to merge 7 commits into from

2 participants

@chesleybrown

That was actually pretty fun. Guess I'll run you through my thought pattern on all this.

Although I was tempted to try the web crawling one I ended up going with the frontend challenge as I enjoy using Angular.

When I initially checked out all the files, my first task was to improve the development workflow. I find this is essential for the start of any project. If the development process itself is restraining you're only making things more difficult for yourself. I'm familiar with grunt and I felt it was the perfect fit here so I began setting that up and picking the technologies I'd be using.

Since this was going to be client side, I knew I needed a package manager for importing any external client-side libraries I wanted (such as Angular, lodash, normalize-css, etc). I also knew I wanted to make the CSS maintainable and easily configureable so I chose SASS and Foundation. And a css normalization file is always a most.

I normally always include lodash because it's just so convenient to have for those moments you need it. I usually find it really speeds up development.

Of course, autocompletes have been done many times before and I'm not one for re-inventing the wheel. I strolled around and found a lightweight autocomplete that worked with Angular. I didn't want any jQuery dependancy for this project as it really just wasn't needed. Too much overhead for no real gain.

After using the angular autocomplete directive I chose I soon discovered it had a few bugs. So I ended up forking the project, fixing the bugs and also putting up pull requests to the original author so everyone could benefit from my fixes. https://github.com/JustGoscha/allmighty-autocomplete/pulls

I started trying to communicate with the API directly from the client but quickly noticed it was suffering from cross-domain restrictions. This wouldn't really be a problem if the project was actually under the busbud domain so I just quickly put together a node proxy to the API.

One thing confused me was the callback function within the API url. I never really witnessed that before, so I just ended up stripping the function name from the result. Feel free to explain why its there? Maybe I'm just missing something...

For the design, I didn't directly want to copy the suggestion provided, but I did use it as a heavy inspiration. And then lastly I created a simple random class background directive to randomize the city background on page load. Felt it made it a bit more friendly that way.

Unfortunately I only have my Mac at home (I normally do all my windows testing on my machines at work) so I haven't tested this in IE. I believe there will be a couple of issues in IE7, but they would be minimal and easy to fix.

If I had more time, I would have liked to add some unit/functional tests as well.

Anyway, let me know what you think.

Cheers,
Ches

@chesleybrown

Forgot to mention that all you have to do is run the grunt command which automatically compiles are the CSS and JS files and starts the node server on port 5000.

@chesleybrown

Sorry, another thing. You'll need to do npm install and bower install.

Let me know if you have any troubles getting the code to run. It's possible I left something else out.

@cdnbacon cdnbacon and 1 other commented on an outdated diff Feb 17, 2014
+ if (!req.param('location')) {
+ res.status(200);
+ res.send(result);
+ }
+
+ var url = 'http://www.busbud.com/en/complete/locations/' + req.param('location') + '?callback=fn';
+
+ var request = http.get(url, function(response) {
+ var buffer = '';
+
+ response.on('data', function(chunk) {
+ buffer += chunk;
+ });
+
+ response.on('end', function(err) {
+ // why is callback fn mandatory? ... stripping it
@cdnbacon
cdnbacon added a note Feb 17, 2014

It's not pure JSON, it's actually JSON-P to allow for cross-domain calls for older browsers.

@chesleybrown
chesleybrown added a note Feb 18, 2014

Ah yes, that makes a lot of sense now. We've dropped support for old versions of IE so I'm use to using CORS instead. I'll update it to use proper JSONP which means I won't need the api proxy at all.

@chesleybrown
chesleybrown added a note Feb 18, 2014

I've updated to use proper JSONP in 98276dc. Let me know what you think.

I've also switched to the original repo for allmighty autocomplete as the author has accepted all my pull request fixes. This means you should do another bower install after you get the latest code. See 3a7c301.

I should get a chance to look at the browser related issues you noted later tonight or tomorrow evening.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@cdnbacon cdnbacon and 1 other commented on an outdated diff Feb 17, 2014
@@ -0,0 +1,12 @@
+{
+ "name": "frontend-challenge",
+ "version": "0.0.1",
+ "dependencies": {
+ "angular" : "1.2.12",
+ "angular-route": "1.2.12",
+ "lodash": "2.4.1",
+ "normalize-css": "3.0.0",
+ "foundation": "5.1.1",
+ "allmighty-autocomplete": "chesleybrown/allmighty-autocomplete"
@cdnbacon
cdnbacon added a note Feb 17, 2014

Nicely done!

@chesleybrown
chesleybrown added a note Feb 18, 2014

Thank you. 😸

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@cdnbacon cdnbacon commented on the diff Feb 17, 2014
web/js/directives/randomclass-directive.js
+angular.module('randomclass-directive', [])
+ .directive('randomClass', function() {
+ return {
+ restrict: 'A',
+ scope: {
+ prefix: '@classPrefix',
+ min: '=',
+ max: '='
+ },
+ link: function(scope, element, attrs) {
+ var num = Math.floor(Math.random() * (scope.max - scope.min + 1)) + scope.min;
+ element.addClass(scope.prefix + num.toString());
+ }
+ }
+ })
+;
@cdnbacon
cdnbacon added a note Feb 17, 2014

Why write this as a client-side module and not something that's spit out from the HTML template for the page?

@chesleybrown
chesleybrown added a note Feb 18, 2014

I'm actually not doing any server side processing. This means we only ever need to serve static files which means exceptionally easy to scale and incredibly lightweight and fast. Although I'm using node to serve these static files for development, if this were to be placed in a production environment I would use something more suitable for serving static files such as nginx (or even a CDN) which will yield much better results. And of course use things such as gzip, js minify, browserify, etc for maximum effect.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@cdnbacon

I grabbed a few screenshots through use of SauceLabs and localtunnel. Here's what it looks like on popular browsers where I noticed issues.

IE7

_onie7

IE8

_onie8

Safari 5

_onsafari5

Safari 6

_onsafari6

@cdnbacon cdnbacon closed this Feb 19, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.