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.

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

WIP refactor getPokestopMarker to show both rocket + reward #548

Closed

Conversation

yw2theorycrafter
Copy link
Contributor

getPokestopMarker has a lot of code duplication, in my opinion. This makes it hard to customize the appearance of pokestops. And, the existing implementation can't show both a rocket head + reward, and in this case it will only show the rocket head - which seems backwards, I'd prefer to see the quest reward!

This PR simplifies the whole thing to take a "merged" approach, where the html field of the marker icon is built up in stages. Rocket heads always go in the lower left, rewards on top of the marker.

TODO: This PR doesn't respect the "noRocket", etc. variables. I'm not sure it needs to, since aren't those things enforced in raw_data.php any way? Either way I didn't do it because I, personally, don't restrict them on my map.

TODO: I don't set the "class" correctly. If it has a rocket I set the rocket class, otherwise I just set the default marker class. I'm not convinced we're getting anything out of the custom CSS classes for the different marker categories, given that the thing you'd really want to customize is the size and placement of the rewards (which is hardcoded in getPokestopMarker...)

Some examples with this PR:
image
image

@kbtbc
Copy link
Contributor

kbtbc commented Sep 1, 2021

Something to further consider, is that until two days ago all Pokestops had two possible quest rewards, depending only if the player had an 'AR Mapping' quest in their inventory or not. Niantic just disabled the 'AR' set of quests, but I expect it will return, and eventually we should be able to display A/B quest rewards as well.

@jepke
Copy link
Collaborator

jepke commented Sep 3, 2021

Would you be able to base this on staging/develop branch as those have a completely rewritten icon approach based on UICONS.

https://github.com/UIcons/UIcons

@yw2theorycrafter
Copy link
Contributor Author

@jepke OK.

Closing in favor of #552.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants