CodeforAustralia / rerouteit

Code For Australia's version of the US ReRoute It transport choices application
MIT License
5 stars 0 forks source link

Ui #14

Closed ghost closed 10 years ago

ghost commented 10 years ago

List of changes

  1. Bootstrap UI
  2. library access to Google Directions
ghost commented 10 years ago

Is Cucumber really necessary? In my experience it becomes a large maintenance burden later on. I'd recommend using something capybara for acceptance tests.

Also it seems like you've bunched 2 issues into one? I believe GitFlow is being practised so you'll want to spilt these contributions into separate pull requests for merging into the develop branch.

ghost commented 10 years ago

My point about Cucumber is that Gherkin can easily become a maintenance burden. Sure it's great to have human readable features but only if there are non-developer stakeholders that want to verify them. In practice I've found that stakeholders rarely care about human readable specs, with the cucumber scenarios not being read by anyone but the developers.

I'd say it'd be much simpler to just use RSpec/Test::Unit and Capybara. Thoughts @cdaloisio?

cdaloisio commented 10 years ago

Thanks guys for your inputs!

@Soliah I agree and would also recommend using RSpec also as it has a very readable syntax, and as we don't have any stakeholders who will be viewing the code, it will be easier to just write tests that other programmers can understand without the extra layer of Cucumber matches.

@arvindcj It will be a good idea I think to keep every issue in a separate branch and name them accordingly. Eg. Issue 3. Implement class to communicate with Google Directions API would become a branch feature/3-google-api-classes

This way when managing pull requests, it's all nicely organised :smile:

I'll review the pull request and make a few notes on it later today.

cdaloisio commented 10 years ago

Also when creating a pull request, it will default to the master branch, so make sure that you change it to merge with develop instead. We will then review and merge develop into master at the end of each sprint.

ghost commented 10 years ago

This PR also adds AngularJS. Is this going to be a single page app without server side HTML rendering?

ghost commented 10 years ago

Christopher,

it is going to be a single page app with server side HTML.

Thanks, Arvind

On Mon, Sep 8, 2014 at 8:47 PM, Christopher Chow notifications@github.com wrote:

This PR also adds AngularJS. Is this going to be a single page app without server side HTML rendering?

— Reply to this email directly or view it on GitHub https://github.com/CodeforAustralia/rerouteit/pull/14#issuecomment-54801969 .

ghost commented 10 years ago

@arvindcj That doesn't make sense unless you miss typed. AngularJS is a full client side UI rendering framework. Templates should be rendered on the client side and the server should be relegated to just being an API, decoupling the backend from the frontend.

This will raise problems with SEO if that is the architecture that is being decided upon unless we lean upon something like https://prerender.io/.

ghost commented 10 years ago

Just explaining the User interface a bit more.

It will be a single page app with a static form served server-side. when the user inputs the "origin" and "Destination" there will be some dynamic html generated to serve the "direction" information.

Do you have any inputs ?

On Mon, Sep 8, 2014 at 9:23 PM, arvind janakiram code@arvindcj.com wrote:

Christopher,

it is going to be a single page app with server side HTML.

Thanks, Arvind

On Mon, Sep 8, 2014 at 8:47 PM, Christopher Chow <notifications@github.com

wrote:

This PR also adds AngularJS. Is this going to be a single page app without server side HTML rendering?

— Reply to this email directly or view it on GitHub https://github.com/CodeforAustralia/rerouteit/pull/14#issuecomment-54801969 .

ghost commented 10 years ago

Ok, because a single page app usually means a thin JS client backed by a JSON API. So when you included AngularJS I inferred that's what you meant.

Are you planning on using AngularJS to do the client side behaviour? Because that seems like overkill for a simple content replacement, especially since the HTML is being rendered server side. You could potentially achieve the same result simply using jQuery and .html() though maybe I'm missing something.

After all, this sounds like we're gonna lose all the nice stuff that something like Angular would give (routing, data binding etc).

ghost commented 10 years ago

@Soliah ok i understand where you are coming from.

There are two options that are available to us - 1) Client side - calls to the "Google Directions API" where the entire app is run from the browser. Angular JS made a lot of sense here.

2) Server Side - here the calls to "Google Directions API" are from the server. we are going with this approach.

I will be removing Angular JS as it is not required. I intended to do it client side first. But @cdaloisio and I decided to go with option 2.

Will set some time aside tonight to incorporate the changes.

Thanks Again, Arvind

cdaloisio commented 10 years ago

@arvindcj I am going to close this request as I have created another pull request that includes your commits, but have updated some formatting, etc. You can have a look at #21 for the details :smile: