Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor POIs so that Mobs can be displayed #131

Merged
merged 24 commits into from
Oct 19, 2016
Merged

Refactor POIs so that Mobs can be displayed #131

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
055d9b2
Make mapPokePOI() a utility function instead of a static method
MajorBreakfast Oct 17, 2016
d8772f0
Call constructor, remove unusual Object.create() usage
MajorBreakfast Oct 17, 2016
fef4f67
Add button to manually show mob card
MajorBreakfast Oct 18, 2016
b9f78ac
Make poi-card compatible with sightings and mobs
MajorBreakfast Oct 18, 2016
798ae98
Merge branch 'develop' of https://github.com/PokemonGoers/Catch-em-al…
MajorBreakfast Oct 18, 2016
23b313f
Rename PokePOI to POI
MajorBreakfast Oct 18, 2016
0540a55
Rename PokeMob to Mob
MajorBreakfast Oct 18, 2016
4263399
Rename PokePrediction to Prediction
MajorBreakfast Oct 18, 2016
d112e48
Rename PokeSighting to Sighting
MajorBreakfast Oct 18, 2016
d86c856
Rename PokeGender to PokemonGender, PokeAttackCategory to PokemonAtta…
MajorBreakfast Oct 18, 2016
39c9960
Remove pokemon property from Sighting and Prediction
MajorBreakfast Oct 18, 2016
55ca79e
Refactor POI and its subclasses
MajorBreakfast Oct 18, 2016
1dd9ada
Replace Object.assign
MajorBreakfast Oct 18, 2016
101f2d0
PokeMob logo
MajorBreakfast Oct 18, 2016
0e503a9
Refactor poi-card layout and integrate mobs
MajorBreakfast Oct 18, 2016
6688ee6
Artificial tweets
MajorBreakfast Oct 18, 2016
84414db
Make POI#type a value property
MajorBreakfast Oct 18, 2016
451ffa6
Make tweet count text plural only
MajorBreakfast Oct 19, 2016
94e5d7a
Use elvis operator
MajorBreakfast Oct 19, 2016
569202d
Set Mob data from map data, refactor MobTweet type
MajorBreakfast Oct 19, 2016
73279c6
Extract poiFromMapEventData() into a module
MajorBreakfast Oct 19, 2016
ae01cc1
Show artifial mob button only in development mode
MajorBreakfast Oct 19, 2016
6599949
Use ngIf instead of hidden attribute
MajorBreakfast Oct 19, 2016
f8ace07
get isDevelopEnvironment()
MajorBreakfast Oct 19, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion ionic2/app/pages/map/map.page.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,5 +12,8 @@
<poke-poi-card #poiCard></poke-poi-card>

<button id="artificial-mob-button"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@MajorBreakfast MajorBreakfast Oct 19, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@WoH WoH Oct 19, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@WoH What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • 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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe show it only in dev Mode?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now it is only shown in dev mode

(click)="showArtificialMob()">Show artificial mob card</button>
(click)="showArtificialMob()"
[hidden]="config.buildEnvironment !== 'develop'">
Show artificial mob card
</button>
</ion-content>
2 changes: 1 addition & 1 deletion ionic2/app/pages/map/map.page.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ export class MapPage {
positionLoaded: Promise<any> = null;

constructor(private navParams: NavParams,
private config:ConfigService,
private config: ConfigService,
private popoverCtrl: PopoverController,
private events: Events,
private filterService: FilterService) {
Expand Down
2 changes: 1 addition & 1 deletion ionic2/app/services/config.service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import env from '../env';

@Injectable()
export class ConfigService {

get apiEndpoint(): string {
if (env.BUILD_TARGET !== 'web' && env.API_ENDPOINT) {
return env.API_ENDPOINT;
Expand All @@ -25,4 +24,5 @@ export class ConfigService {
}
}

buildEnvironment = env.BUILD_ENV;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

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

For consistency reasons this is better here maybe.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. Even better: a method isDevelopEnvironment()

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

But that's just my personal preference.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

}