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

Marker Icons #87

Merged
merged 8 commits into from
Apr 13, 2020
Merged

Marker Icons #87

merged 8 commits into from
Apr 13, 2020

Conversation

tommypoa
Copy link
Contributor

What's new in this PR

  • Added icons for store markers – resting and focused state
  • Added icon for current location
  • Animation for clicking store markers implemented – map region will pan to the store

Relevant Links

  • N/A

How to review

  • Code quality + Figma

Next steps

  • Implement smooth animation for transition between resting and focused state of store markers (see gif below)
  • White outline for store name beneath the markers

Tests Performed, Edge Cases

Gif

Screen-Recording-2020-04-13-at-1

@wangannie wangannie self-requested a review April 12, 2020 19:55
Copy link
Member

@wangannie wangannie left a comment

Choose a reason for hiding this comment

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

I noted a few styling changes on the map icons. The images definitely look a little pixelated, so update these with higher-res images from Ace. Also, we should maybe look into using svgs (check out this guide).

The map animations are still super jumpy. It seems to work well from a closer zoom level but not so well from far away. https://www.loom.com/share/c1693f7b60af4d4d8825c0653bdc54e9 I'm not sure if it may be better just to not include them at all until we can figure out how to make it smooth.
Minor detail but I'm also getting this weird jumping https://www.loom.com/share/3e8965da9df34afcba89c1660cc70964

Future todo to reduce clutter:

  • Only show the marker label at a certain zoom level. If it's too zoomed, out, only show the icons.

components/store/StoreMarker.js Outdated Show resolved Hide resolved
styled/store.js Show resolved Hide resolved
styled/store.js Outdated Show resolved Hide resolved
return (
<MarkerContainer>
{focused && (
<Image source={require('../../assets/images/Marker_Focused.png')} />
Copy link
Member

Choose a reason for hiding this comment

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

Hmm probably need higher resolution versions of these

Copy link
Member

@wangannie wangannie left a comment

Choose a reason for hiding this comment

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

Looking good!
I just noticed this issue sometimes where it lets me have 2 stores selected at once. It usually clears after tapping another store again. I'm not sure if it maybe has something to do with setting the default location? Going to go ahead and merge this now, but definitely something to keep an eye on when you make the next map marker updates.
IMG_3109

@wangannie wangannie merged commit 34dcc4d into master Apr 13, 2020
@wangannie wangannie deleted the tommy/markers branch April 13, 2020 05:28
@wangannie wangannie restored the tommy/markers branch April 13, 2020 05:28
@annieyro annieyro deleted the tommy/markers branch April 17, 2020 04:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants