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

566 amenity marker active states #571

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

abinocoe
Copy link
Contributor

@abinocoe abinocoe commented Jul 4, 2018

Fixes #566 implemented active states for amenity markers. Also clear stage card when amenity marker is selected.

simulator screen shot - iphone 7 - 2018-07-04 at 11 24 08

Pre-merge check-list

  • Tester approved

Tester check-list

  • Tested on multiple devices / OS versions (Test on the physical device(s) you have access to, otherwise use BrowserStack):

    • Galaxy S8 (Android 7.0/8.0)
    • Galaxy S7 (Android 6.0)
    • iPhone X (iOS 11.x)
    • iPhone 8 (11.x)
    • iPhone 7 (iOS 10.x)

    Optional:

    • Google Pixel 2
    • Galaxy S8 Plus
    • Galaxy S7 Edge
    • Galaxy S6
    • iPhone 7+
    • iPhone 6

Accessibility Testing (If applicable)

  • Text-to-Voice (Android: Voice Assistant / iOS: Speak Selection)
  • Large Text (=<200%)
  • Landscape Screen orientation

Weak connection testing (If applicable)

  • Airplane mode
  • 2G/3G Network Settings

@abinocoe abinocoe self-assigned this Jul 4, 2018
RGBboy
RGBboy previously requested changes Jul 6, 2018
Copy link
Contributor

@RGBboy RGBboy left a comment

Choose a reason for hiding this comment

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

Looks good. Just would like a single test for active amenity marker.

markerSelect={() => {}}
handleMarkerPress={() => {}}
activeMarker={undefined}
/>
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add one test for active amenity too please?

@RGBboy RGBboy assigned RGBboy and unassigned abinocoe Jul 9, 2018
@RGBboy RGBboy dismissed their stale review July 9, 2018 17:09

Updated and need others to review

Copy link
Contributor

@frigus02 frigus02 left a comment

Choose a reason for hiding this comment

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

Looks mostly good to me. I would like to keep it working on Windows, though. (Although we already broke it in other ways, but these are easy to fix.)

@@ -106,9 +106,6 @@
"transformIgnorePatterns": [
"node_modules/(?!(jest-)?react-native|react-navigation)"
],
"moduleNameMapper": {
"\\.(png|gif|jpeg|jpg)$": "RelativeImageStub"
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Unfortunately this piece seems to be needed to run the tests on Windows. Otherwise you get errors like this when an image is loaded in a test:

 FAIL  src/screens/FilterScreen/Header.test.js
  ● Test suite failed to run

    C:/code/pride-london-app/assets/images/chevron-left-white.png: Unexpected character '�' (1:0)

      > 1 | �PNG
          | ^
        2 | →
        3 |
        4 | IHDR   0   ♠   W☻��   ☺sRGB ��∟�   �IDATh♣�I

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants