Refactor POIs so that Mobs can be displayed #131

MajorBreakfast · 2016-10-18T14:13:57Z

~~WIP~~

…into mob-card

…ckCategory

MajorBreakfast · 2016-10-18T18:28:09Z

Note: This PR is a lot more than this simple layout. I've refactored the layout of the sightings card and a lot of related stuff.

You may review it now! :) Let's get it merged

gyachdav · 2016-10-18T19:08:41Z

Mucho cool!

WoH · 2016-10-19T01:13:09Z

There's 2 or more things going on in one PR again ...

MajorBreakfast · 2016-10-19T05:38:03Z

more things going on in one PR

@WoH There's not. I never touched anything that wasn't related. If you want, you can think about this PR as "Refactor POIs so that Mobs can be displayed". If I had left things as they were, it would have been a mess. Nevertheless, plz review. I'd like to get this in. ;)

WoH · 2016-10-19T05:58:32Z

@MajorBreakfast I would've liked to have the initial refractoring in a PR.

WoH · 2016-10-19T06:04:00Z

I'd want @johartl to comment, the POI Cards are his baby.

MajorBreakfast · 2016-10-19T08:07:27Z

I would've liked to have the initial refractoring in a PR.

@WoH I've changed the name. The actual Mob card is the least part of this PR. However, there is little value in separating it out. I agree that @johartl should comment.

MajorBreakfast · 2016-10-19T11:32:33Z

I'm going to merge this this evening unless there are objections to any of the things I changed. The code is clean and I tested all affected app parts thoroughly, so it should be fine.

johartl · 2016-10-19T12:12:17Z

ionic2/app/components/map/map.component.ts

+    return sighting;
+  } else if ('clusterId' in rawData) {
+    const mob = new Mob();
+    return mob;


What's the reason for returning an empty mob object here?

Does the map already produce mobs? If so, what's the exact format of the event data?

I'm not sure if they already do. But the data should look like this PokemonGoers/PokeMap-2#32 (comment) which is exactly what our Mob class represents.
The best thing you can probably do is const mob = Mob.fromObject(rawData);

Well I just realized that you've removed the fromObject method.

I removed it from the model itself because it doesn't really belong in there.

johartl · 2016-10-19T12:20:29Z

ionic2/app/components/poi-card/poi-card.component.html

+      <div class="title">PokeMob</div>
+      <div class="description">
+        A lot of players seem to gather at this location!<br><br>
+        Based on {{ poi.tweets.length }} tweet(s).


By the definition of a mob there should always be multiple tweets so I think you can replace tweet(s) by tweets

Okay. I'll change it.

johartl · 2016-10-19T12:40:06Z

ionic2/app/pages/map/map.page.html

@@ -10,4 +10,7 @@
  </button>

  <poke-poi-card #poiCard></poke-poi-card>
+
+  <button id="artificial-mob-button"


I'm glad you added this button so I can test your functionality. But obviously that shouldn't end up in the final product. So I think either the button should be removed at all or somehow hidden such that it can only by used by developers.

Let's leave it in for now, so that people can try it out. It's easy enough to remove later. It's for developers only, but triggering it through the console would be two cumbersome.

Just make sure neither mock data nor the button remain after all.
I like being able to test it that way though.
This would be beautiful to test with a mock websocket (I believe that's where we get those).

How is triggering it through the console cumbersome? And even if it is, this functionality will only ever be used for testing purposes by developers. So it doesn't have to be easily accessible by everyone.

@WoH What do you think?

I can remove it immediately, e.g. by uncommenting it

Or keep it for a while, e.g. remove it before we launch/map shows mobs

Maybe show it only in dev Mode?

Maybe set the style to display: none and then anyone can still look for the element in the DOM using the developer console and make it visible.

Now it is only shown in dev mode

johartl · 2016-10-19T12:45:12Z

Regarding the discussion about too many things in one PR. I mostly agree that all changes focus around the mob card with the exception of renaming PokeSighting, PokeAttack, PokeGender, PokeMob, and some more. Personally, I think you could have done this in a separate PR.

WoH · 2016-10-19T13:19:29Z

ionic2/app/components/map/map.component.ts

-    } else {
-      throw new Error('PokePOI cannot be identified as PokeSighting or PokeMob:\n' + JSON.stringify(pokePOI));
-    }
+function poiFromMapEventData(rawData: any) : POI {


no reason to use the function keyword

Is that TypeScript feature?

Yes, and we are using it all over the place I think 😆 http://www.typescriptlang.org/docs/handbook/classes.html

Compile error. Am I doing something wrong?

Can you paste the error?

TypeScript docu uses the keyword as well https://www.typescriptlang.org/docs/handbook/functions.html

The class ends before the function definition, I didn't realiize this on github.

Yep, exactly.

@johartl Should I make this a static method or a utility function? What do you prefer? I and @WoH need another opinion

I prefer the static method

WoH · 2016-10-19T13:20:56Z

ionic2/app/components/poi-card/poi-card.component.html

-    <ion-row>
+<div id="slide-card" #slideCard @slide="slideState">
+  <!-- Sightings -->
+  <div *ngIf="poi && poi.type == 'sighting' && pokemon">


you could make use of the Elvis operator here: <div *ngIf="poi?.type == 'sighting' && pokemon">

Elvis operator :) I'll change that

I know that's something very Angular2 specific, but I think it is nice. Easy to understand, and makes these checks really concise.

WoH · 2016-10-19T13:21:41Z

ionic2/app/components/poi-card/poi-card.component.html

-            Directions
-          </button>
+  <!-- Mobs -->
+  <div *ngIf="poi && poi.type == 'mob'">


johartl · 2016-10-19T14:10:50Z

ionic2/app/models/mob.ts

  timestamp: number;

  type = 'mob';
 }

-export type PokeTweet = {
+export type MobTweet = {


If we extract this class to a separate file and call it Tweet we can reuse it in other parts of the application. For instance, the sentiment analysis also returns an array of tweets and these are not related to mobs.

MajorBreakfast · 2016-10-19T15:02:49Z

@johartl Plz update your review.

MajorBreakfast · 2016-10-19T15:48:10Z

Clear to merge? Any other wishes?

WoH · 2016-10-19T16:08:45Z

ionic2/app/services/config.service.ts

@@ -25,4 +24,5 @@ export class ConfigService {
    }
  }

+  buildEnvironment = env.BUILD_ENV;


get buildEnvironment(): string { return env.BUILD_ENV; }

For consistency reasons this is better here maybe.

I agree. Even better: a method isDevelopEnvironment()

get isDevelopEnvironment(): boolean { return env.BUILD_ENV === 'develop'; }

But that's just my personal preference.

MajorBreakfast added 16 commits October 17, 2016 20:20

Make mapPokePOI() a utility function instead of a static method

055d9b2

Call constructor, remove unusual Object.create() usage

d8772f0

Add button to manually show mob card

fef4f67

Make poi-card compatible with sightings and mobs

b9f78ac

Merge branch 'develop' of https://github.com/PokemonGoers/Catch-em-all …

798ae98

…into mob-card

Rename PokePOI to POI

23b313f

Rename PokeMob to Mob

0540a55

Rename PokePrediction to Prediction

4263399

Rename PokeSighting to Sighting

d112e48

Rename PokeGender to PokemonGender, PokeAttackCategory to PokemonAtta…

d86c856

…ckCategory

Remove pokemon property from Sighting and Prediction

39c9960

Refactor POI and its subclasses

55ca79e

Replace Object.assign

1dd9ada

PokeMob logo

101f2d0

Refactor poi-card layout and integrate mobs

0e503a9

Artificial tweets

6688ee6

Make POI#type a value property

84414db

MajorBreakfast mentioned this pull request Oct 18, 2016

Mob card #80

Closed

MajorBreakfast changed the title ~~Mob card~~ Refactor POIs so that Mobs can be displayed Oct 19, 2016

johartl requested changes Oct 19, 2016

View reviewed changes

Make tweet count text plural only

451ffa6

WoH reviewed Oct 19, 2016

View reviewed changes

MajorBreakfast added 2 commits October 19, 2016 15:34

Use elvis operator

94e5d7a

Set Mob data from map data, refactor MobTweet type

569202d

johartl reviewed Oct 19, 2016

View reviewed changes

MajorBreakfast added 2 commits October 19, 2016 17:35

Extract poiFromMapEventData() into a module

73279c6

Show artifial mob button only in development mode

ae01cc1

Use ngIf instead of hidden attribute

6599949

WoH approved these changes Oct 19, 2016

View reviewed changes

get isDevelopEnvironment()

f8ace07

johartl approved these changes Oct 19, 2016

View reviewed changes

MajorBreakfast merged commit 2c6511e into PokemonGoers:develop Oct 19, 2016

Refactor POIs so that Mobs can be displayed #131

Refactor POIs so that Mobs can be displayed #131

Conversation

MajorBreakfast commented Oct 18, 2016 • edited Loading

MajorBreakfast commented Oct 18, 2016 • edited Loading

gyachdav commented Oct 18, 2016

WoH commented Oct 19, 2016 • edited Loading

MajorBreakfast commented Oct 19, 2016 • edited Loading

WoH commented Oct 19, 2016

WoH commented Oct 19, 2016

MajorBreakfast commented Oct 19, 2016 • edited Loading

MajorBreakfast commented Oct 19, 2016

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MajorBreakfast Oct 19, 2016 • edited Loading

Choose a reason for hiding this comment

WoH Oct 19, 2016 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johartl commented Oct 19, 2016

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MajorBreakfast Oct 19, 2016 • edited Loading

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

WoH Oct 19, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johartl Oct 19, 2016 • edited Loading

Choose a reason for hiding this comment

MajorBreakfast commented Oct 19, 2016

MajorBreakfast commented Oct 19, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MajorBreakfast commented Oct 18, 2016 •

edited

Loading

MajorBreakfast commented Oct 18, 2016 •

edited

Loading

WoH commented Oct 19, 2016 •

edited

Loading

MajorBreakfast commented Oct 19, 2016 •

edited

Loading

MajorBreakfast commented Oct 19, 2016 •

edited

Loading

MajorBreakfast Oct 19, 2016 •

edited

Loading

WoH Oct 19, 2016 •

edited

Loading

MajorBreakfast Oct 19, 2016 •

edited

Loading

WoH Oct 19, 2016 •

edited

Loading

johartl Oct 19, 2016 •

edited

Loading