ISS Location IA #1559

javathunderman · 2015-02-21T19:21:36Z

Continuation of #1217. Please refer to the PR template there for more information!

This PR fixes a few things that were not in the old one.

Please let me know if I need to update this comment!

cc @mintsoft @jagtalon @moollaza @MrChrisW @rishiag

IA Page: https://duck.co/ia/view/isslocation

…content.handlebars

…isslocation.css

javathunderman · 2015-02-22T14:05:27Z

@moollaza Is there anything in particular I need to add here?

jagtalon · 2015-02-23T19:02:16Z

@javathunderman thanks! We're checking it out!

jagtalon · 2015-02-23T19:24:54Z

share/spice/isslocation/isslocation.js

+        var issLatitude = (Math.round(api_result.iss_position.latitude * 100) / 100).toFixed(2);
+        var issLongitude = (Math.round(api_result.iss_position.longitude * 100) / 100).toFixed(2);
+
+        DDG.require('maps', function()


Missing opening curly braces.

@jagtalon What line?

jagtalon · 2015-02-23T19:30:38Z

lib/DDG/Spice/ISSLocation.pm

+attribution github => ['https://github.com/rishiag','Rishi Agarwal'],
+           twitter => ['http://twitter.com/rishiagar','Rishi Agarwal'];
+
+triggers any => "iss location", "iss position";


Other triggers might be "international space station" or maybe "iss tracker / iss tracking" as mentioned in #1217 (comment)

javathunderman · 2015-02-23T19:31:07Z

@jagtalon Fixed a few issues, but not sure if that's what you meant.

jagtalon · 2015-02-23T19:36:24Z

share/spice/isslocation/isslocation.js

+            model: 'Place',
+
+            view: 'Map',
+            data: [{


Adding url here should make the link on the map work.

data: [{ url: "...", ... }]

@jagtalon How does that work? Do you mean I should add

url: "(link goes here)",

javathunderman · 2015-02-23T19:55:24Z

@jagtalon There's an issue that's making the tests fail; I think it's some issue with Travis, not this code, as a lot of old IAs are failing in the error report.

jagtalon · 2015-02-23T21:09:28Z

@javathunderman ah I see--don't worry about the tests for now. It's working on my end anyway

jagtalon · 2015-02-23T21:12:21Z

@javathunderman looking forward to your changes!

jagtalon · 2015-02-23T21:25:30Z

share/spice/isslocation/isslocation.js

+    "use strict";
+    env.ddg_spice_isslocation = function(api_result) {
+
+       if(!api_result || api_result !== "success") {


This exits because you're comparing api_result with "success", but you should be comparing api_result.message instead.

So I replace both instances with "api_result.message"?

Just the latter one. !api_result says, "check if api_result is not null or undefined" and api_result.message !== "success" says that the message property of api_result should equal to "success" to continue.

Looking at http://your-codio-instance:5000/js/spice/isslocation/ should make it clearer. :)

@javathunderman probably best to check

if (!api_result || !api_result.message || api_result.message !== "success"){

If we know the message is always there, you can leave out the check for the existance of api_result.message

@moollaza Ok, will update tonight.

jagtalon · 2015-02-23T21:33:56Z

I think the tab name should be just "Maps" instead of "ISSLocation". What do you think @abeyang @moollaza @MrChrisW @mintsoft?

mintsoft · 2015-02-23T21:44:50Z

@jagtalon I agree, ISSLocation looks like it's accidentally not set it or something

javathunderman · 2015-02-23T21:45:52Z

@jagtalon Maps is OK with me.

jagtalon · 2015-03-02T18:28:48Z

Alright cool all we need is the asset. Thanks @moollaza

javathunderman · 2015-03-03T02:09:30Z

@jagtalon Just waiting on an icon from @abeyang and I'll rewrite that CSS.

mintsoft · 2015-03-03T19:05:19Z

@abeyang no pressure ;)

jagtalon · 2015-03-04T19:38:25Z

@javathunderman in the mean time, you can apply this change--the JS doesn't work at the moment. #1559 (comment)

javathunderman · 2015-03-06T16:25:37Z

@jagtalon Sorry for the late response- fixed the JS code.

abeyang · 2015-03-06T23:43:13Z

Can we try these on for size? Attaching both 1x and 2x.

Just to know, the exact coordinates should be where the shadow is, not the dot.

javathunderman · 2015-03-07T16:00:57Z

@jagtalon Doesn't seem like that JS code is working...

@abeyang Implementing this...

javathunderman · 2015-03-07T16:06:33Z

@moollaza Not seeing that CSS code in any of the files...Where do I replace the pin with that new asset?

mintsoft · 2015-03-07T16:26:05Z

@javathunderman he's just saying that it should be added to the existing .css file, so that it overrides the styles provided by the inbuilt stylesheets

mintsoft · 2015-03-07T16:58:32Z

@javathunderman just tried it out with this:

.mapview-marker.mapview-marker-star,
.mapview-marker.mapview-marker-star.has-focus {
    background-image: url("1x.png") !important;
    background-size: 30px 44px !important;

    width: 30px !important;
    height: 44px !important;
    margin-left: -15px !important;
}

javathunderman · 2015-03-07T18:53:58Z

Ah, thanks. Implementing this…

share/spice/isslocation/1x.png

javathunderman · 2015-03-10T18:26:18Z

@abeyang Implemented that new icon.

javathunderman · 2015-03-11T20:35:05Z

@jagtalon Anything else to do here?

moollaza · 2015-03-12T00:27:46Z

@javathunderman we'll take a look and get back to you soon

javathunderman · 2015-03-12T20:43:06Z

@moollaza Small issue- if you move the map so far that you come back to one end of the world, the map pin doesn't show up. Should this be as is?

moollaza · 2015-03-17T04:37:47Z

Small issue- if you move the map so far that you come back to one end of the world, the map pin doesn't show up. Should this be as is?

@javathunderman that's a great find -- I think for now we'll consider that tolerated as it seems to be the expected behaviour from Leaflet.js -- Perhaps you should file a bug over there? I'm sure other have noticed this!

moollaza · 2015-03-17T04:41:12Z

@javathunderman @mintsoft I made a mistake -- we can't simply drop in the name of the asset to the CSS because the share files require versioned paths.

We'll need to modify the background image using JS and get_asset_path() to generate the path to the image.

javathunderman · 2015-03-17T10:26:15Z

That CSS worked for sometime until it decided to break…This is probably why. What’s the code I would use? @moollaza @mintsoft

jagtalon · 2015-03-23T20:03:37Z

Checking it out!

jagtalon · 2015-03-23T20:45:52Z

@javathunderman just add the icon in this PR and everything should be good. Maybe we could just use 2x.png for now. Other than that, https://ddh9.duckduckgo.com/?q=iss+location&ia=map

jagtalon · 2015-03-23T22:10:52Z

@javathunderman actually, we got it. Opened a PR here #1675. Thanks for the contribution! :)

ISS Location IA

javathunderman · 2015-03-23T22:30:20Z

@jagtalon no problem. 😄

cc @rishiag Thanks for making the original PR in in #1217!

moollaza · 2015-04-14T19:39:14Z

@javathunderman @rishiag congrats! It's live: https://duckduckgo.com/?q=iss+location&ia=map

Sorry for the delay, it's actually been live for about a week.

Thanks again for taking the time to contribute! We really appreciate it. Feel free to stick around and comment/help on other PR's & Issues or even submit more Instant Answers!

javathunderman · 2015-04-15T20:39:22Z

Yay!

javathunderman added 5 commits February 21, 2015 14:14

Update and rename lib/DDG/Spice to lib/DDG/Spice/ISSLocation.pm

b816af4

Update and rename t to t/ISSLocation.t

5efe617

Update and rename share/spice to share/spice/isslocation/isslocation.js

69b66d5

Update and rename share/spice/isslocation to share/spice/isslocation/…

f7aa605

…content.handlebars

Update and rename share/spice/isslocation to share/spice/isslocation/…

498e803

…isslocation.css

moollaza added New Instant Answer Status: Needs Community Review labels Feb 21, 2015

moollaza mentioned this pull request Feb 21, 2015

ISS Location Spice #1217

Closed

javathunderman mentioned this pull request Feb 22, 2015

WIP: forvo audio with single item #850

Closed

jagtalon reviewed Feb 23, 2015
View reviewed changes

Fixing few issues.

98a35d7

jagtalon reviewed Feb 23, 2015
View reviewed changes

Adding additional triggers.

7cc4330

jagtalon reviewed Feb 23, 2015
View reviewed changes

jagtalon self-assigned this Feb 23, 2015

jagtalon reviewed Feb 23, 2015
View reviewed changes

Delete content.handlebars

1ee9120

Fixed JS code

a0c4c79

modified: share/spice/isslocation/isslocation.css

de5890d

share/spice/isslocation/1x.png

jagtalon pushed a commit that referenced this pull request Mar 23, 2015

Merge pull request #1559 from javathunderman/master

ccf06b3

ISS Location IA

jagtalon merged commit ccf06b3 into duckduckgo:master Mar 23, 2015

moollaza removed the Status: Needs Community Review label Apr 14, 2015

moollaza removed the Status: Work In Progress label Aug 20, 2015

ISS Location IA #1559

ISS Location IA #1559

Conversation

javathunderman commented Feb 21, 2015

javathunderman commented Feb 22, 2015

jagtalon commented Feb 23, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javathunderman commented Feb 23, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javathunderman commented Feb 23, 2015

jagtalon commented Feb 23, 2015

jagtalon commented Feb 23, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jagtalon commented Feb 23, 2015

mintsoft commented Feb 23, 2015

javathunderman commented Feb 23, 2015

jagtalon commented Mar 2, 2015

javathunderman commented Mar 3, 2015

mintsoft commented Mar 3, 2015

jagtalon commented Mar 4, 2015

javathunderman commented Mar 6, 2015

abeyang commented Mar 6, 2015

javathunderman commented Mar 7, 2015

javathunderman commented Mar 7, 2015

mintsoft commented Mar 7, 2015

mintsoft commented Mar 7, 2015

javathunderman commented Mar 7, 2015

javathunderman commented Mar 10, 2015

javathunderman commented Mar 11, 2015

moollaza commented Mar 12, 2015

javathunderman commented Mar 12, 2015

moollaza commented Mar 17, 2015

moollaza commented Mar 17, 2015

javathunderman commented Mar 17, 2015

jagtalon commented Mar 23, 2015

jagtalon commented Mar 23, 2015

jagtalon commented Mar 23, 2015

javathunderman commented Mar 23, 2015

moollaza commented Apr 14, 2015

javathunderman commented Apr 15, 2015