Projectorville #30

michaelpelletier · 2016-01-13T16:15:53Z

Will close #29 when it is done.

First looked like this:

Then a style overhaul happened when I had the idea to make it look like a News Broadcast:

And continuing with progress.

michaelpelletier · 2016-01-16T02:59:09Z

app/controllers/api/api_controller.rb

+          "change" => countries_income_change[country]
+        }
+
+        @countries_data.push(this_country)


Been a long damn time since I've written Ruby and I was never particularly versed in it so I'm not super confident on this being the best way to do this. It also no doubt has broken some tests that I still need to address.

FifthSurprise · 2016-03-08T01:15:28Z

@michaelpelletier Just a reminder to give me a ping on either on Slack or on Github when you are done with this so I can review.

michaelpelletier · 2016-03-08T02:17:18Z

@FifthSurprise Will do! You can always start reviewing it early if you would like.

michaelpelletier · 2016-03-09T16:09:55Z

app/views/layouts/application.html.erb

@@ -3,10 +3,15 @@
  <head>
    <title>WatchTheSkiesDashboard</title>
    <%= stylesheet_link_tag    'application', media: 'all'%>
+
+    <script src="https://cdnjs.cloudflare.com/ajax/libs/angular.js/1.4.3/angular.js"></script>


Note to self: Figure out why Bower stopped playing nicely with Angular.
It's probably fine to use this CDN approach, but if that's the case, let's rip the logic out elsewhere.

FifthSurprise · 2016-03-24T03:32:04Z

Not sure why but it looks like you can lose countries from the dashboard. What I think is happening is on page load, you try to fit all of the tickers into one line but if there's no space, additional countries get pushed below visible window height and then do not appear even after manipulating resizing the window.

FifthSurprise · 2016-03-24T03:33:48Z

app/views/games/dashboard.html.erb

+    <ul>
+      <li>
+        <div ng-repeat="country in countries" class="country">
+          <span>{{country.name}}</span>


I'm not sure if I like Country Name, Amount, then Glyphicon. I think maybe a | as a delimiter would be a good idea between countries.

Sure, easy fix.

FifthSurprise · 2016-03-24T03:41:57Z

Oversight on my part but let's update the README to also bower install

FifthSurprise · 2016-03-24T03:45:22Z

I discovered a bug in the api_controller.rb that was affecting you.

income_change = if previous_income.empty?
should be:
income_change = if previous_income.nil? or previous_income.empty?

I will make this change in PR #40 but you can also just put in the above change if you need it. It handles not being able to compare previous income in the opening round against current income since there is no previous income.

michaelpelletier · 2016-03-24T03:47:38Z

@FifthSurprise Yeah I need to fix the bower install thing and figure out why it's being an asshole with Angular. And good catch with the nil thing!

FifthSurprise · 2016-03-24T03:48:37Z

app/assets/javascripts/angular/controllers/dashboard.js

    $interval(function() {
      $scope.getStatus();
    }, 3000);
  }
-]);
+]).animation('.slide-animation', function () {


I'm guessing this stuff is for animating left to right scroll for the ticker? Some comments could be useful here to that effect.

It's for the news itself, not the stock ticker thing, which definitely means it needs comments.

FifthSurprise · 2016-03-24T03:51:24Z

@michaelpelletier Done looking at this for now.

I'd be interested in maybe throwing a small error image somewhere if the API call gets something other than a 200 response just to make it obvious that something weird happened with the API call. Currently, the dashboard API call will have errors but still try to deliver what it can. So you can use that 200 status message as a healthcheck and just display somewhere on the dashboard a small icon if something is going wrong. Or I guess, throw a javascript error if it sees a 500 but that seems more painful.

Otherwise, the ticker resize thing seems to be the only weirdness.

If you figure that out, I'd like to take another look as sanity check before I merge it in.

FifthSurprise · 2016-03-24T04:25:01Z

app/controllers/api/api_controller.rb

-      unless previous_income_list.empty?
-        previous_income_list.each do |pair|
-          countries_calculations[pair[0]] -= pair[1]
+        income_change = if previous_income.empty?


Derp, realized I can't fix this in #40 since bug is right here.

Anyway, please fix haha.

added migration for games to have a localized time zone add control for time zone in set of new time edit next step is to add a user preference for time zone display

…an asshole.

michaelpelletier · 2016-03-31T12:05:13Z

Left to do:

Rebase and fix merge conflicts.
Add small icon somewhere for if the API explodes.
Switch away from the Angular CDN.
Add more comments in the JS
Add | between countries on the news ticker.
Fix weirdness with ticker sizing / resizing.

michaelpelletier · 2016-04-02T14:53:54Z

@FifthSurprise I think this is ready for review now.

I ended up not being able to get bower to play nicely with Angular and Angular-Animate, and per our discussion previously, I don't think a CDN is a good way to go either, so what I did was create a lib folder inside the JS folder and just include the files that way. With Angular's upgrade path to 2.0 being risky (at best), we wouldn't simply want to use Bower to handle any such upgrade anyway (nor have it be a "oh let's just do it" thing). This method also allowed me to easily specify that I wanted to specifically use the minified versions, so we should get a nice little performance gain there, too! I am still not certain that we use Boostrap's JS for anything, but I didn't want to remove it and risk breaking things. I did also replace it with the minified version, though. Small victories.

I added a bunch more comments to the JS, which I hope clear some things up. The dashboard.js file has been almost entirely rewritten, so please do review it and make sure that everything makes sense. Likewise, I think you saw this already, but I did modify the api_controller to give us back some more information about the countries income. It's been ~3 years since I've written any Ruby on Rails and I was never that proficient in the first place, so this could use a good review, too.

FINALLY, I adjusted the get request within the dashboard.js file to have a degree of error handling, so there's now an icon on the dashboard for if there happens to be a connection problem. It may well be a little bit large, but I'm not sure how close you guys typically are to the projector itself, and I wanted to make sure that it would be visible.

FifthSurprise · 2016-04-03T19:48:09Z

Initial look seems like everything is fine. I am going to work on automated testing on rails side in the nearish future so hopefully that will be less of a concern. Also the icon is perfect.

I'm going to poke around a bit more to see if there's anything weird I don't understand but will merge it in soonish.

michaelpelletier · 2016-04-07T02:53:56Z

Yay! 😄

michaelpelletier added the Work-In-Progress label Jan 13, 2016

michaelpelletier reviewed Jan 16, 2016
View reviewed changes

michaelpelletier force-pushed the projectorville branch from f444b68 to 2c46863 Compare February 29, 2016 04:40

michaelpelletier reviewed Mar 9, 2016
View reviewed changes

michaelpelletier added PR Ready for Review Work-In-Progress and removed Work-In-Progress PR Ready for Review labels Mar 14, 2016

FifthSurprise reviewed Mar 24, 2016
View reviewed changes

michaelpelletier force-pushed the projectorville branch from 8b26e47 to 83e7e9d Compare March 31, 2016 11:44

FifthSurprise and others added 7 commits March 31, 2016 07:45

Switch to Chicago time

f8383f6

Update to chicago for real

4f4caf2

Disable tweeting

ea97896

Better message for time and update time zone to California

3c8a68f

working through issue #33

00cce0d

added migration for games to have a localized time zone add control for time zone in set of new time edit next step is to add a user preference for time zone display

Move country data into ticker.

d711ee9

Remove Rioter / Activity section.

f943c25

Michael Pelletier added 7 commits March 31, 2016 07:52

Add support for vertical / horizontal media images for news.

425704e

Let's call this Version 1.

43b2242

Adjust seed data because I forgot.

abab0da

Delete image placeholder.

54fdad0

Include Angular manually because it hates Bower for some reason like …

2b1d45e

…an asshole.

Finish handling news image orientation on the front.

ccd4b0a

Use minified versions of Angular.

12371ed

michaelpelletier force-pushed the projectorville branch from 83e7e9d to 12371ed Compare March 31, 2016 11:53

Missed this during a gross merge conflict.

d0f2b4c

michaelpelletier force-pushed the projectorville branch from b9e287b to d0f2b4c Compare March 31, 2016 12:00

Michael Pelletier added 12 commits April 2, 2016 10:05

Remove empty file.

bff3413

Add comment about news animation.

76f24c3

Rework news ticker to just be a marquee.

c31b82f

Fix bad indenting.

0312af1

Remove angular CDN script tags.

63686a9

Move lib js files into lib folder. use minified versions. forget bower.

33ff1c7

Fix mistake on api_controller due to merge conflicts.

0e4844f

Remove unminified versions.

13d2400

Use minified version of angular timer.

1ab12b3

Remove old image.

83a76e0

Restructure request to allow error handling. Add connection error icon.

dc32ef5

Add comments throughout JS.

1c22c2e

michaelpelletier added PR Ready for Review and removed Work-In-Progress labels Apr 2, 2016

FifthSurprise merged commit 1c22c2e into develop Apr 7, 2016

michaelpelletier deleted the projectorville branch April 7, 2016 02:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Projectorville #30

Projectorville #30

michaelpelletier commented Jan 13, 2016

michaelpelletier Jan 16, 2016

FifthSurprise commented Mar 8, 2016

michaelpelletier commented Mar 8, 2016

michaelpelletier Mar 9, 2016

FifthSurprise commented Mar 24, 2016

FifthSurprise Mar 24, 2016

michaelpelletier Mar 24, 2016

FifthSurprise commented Mar 24, 2016

FifthSurprise commented Mar 24, 2016

michaelpelletier commented Mar 24, 2016

FifthSurprise Mar 24, 2016

michaelpelletier Mar 24, 2016

FifthSurprise commented Mar 24, 2016

FifthSurprise Mar 24, 2016

michaelpelletier commented Mar 31, 2016

michaelpelletier commented Apr 2, 2016

FifthSurprise commented Apr 3, 2016

michaelpelletier commented Apr 7, 2016

Projectorville #30

Projectorville #30

Conversation

michaelpelletier commented Jan 13, 2016

michaelpelletier Jan 16, 2016

Choose a reason for hiding this comment

FifthSurprise commented Mar 8, 2016

michaelpelletier commented Mar 8, 2016

michaelpelletier Mar 9, 2016

Choose a reason for hiding this comment

FifthSurprise commented Mar 24, 2016

FifthSurprise Mar 24, 2016

Choose a reason for hiding this comment

michaelpelletier Mar 24, 2016

Choose a reason for hiding this comment

FifthSurprise commented Mar 24, 2016

FifthSurprise commented Mar 24, 2016

michaelpelletier commented Mar 24, 2016

FifthSurprise Mar 24, 2016

Choose a reason for hiding this comment

michaelpelletier Mar 24, 2016

Choose a reason for hiding this comment

FifthSurprise commented Mar 24, 2016

FifthSurprise Mar 24, 2016

Choose a reason for hiding this comment

michaelpelletier commented Mar 31, 2016

michaelpelletier commented Apr 2, 2016

FifthSurprise commented Apr 3, 2016

michaelpelletier commented Apr 7, 2016