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

bug fix: opening soon #149

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

Conversation

alyiri
Copy link
Contributor

@alyiri alyiri commented Jun 9, 2020

What

Pull opening soon logic out into a function, use the new variables from locations data.

Why

My previous PR to show the hours as a range (#128) introduced a bug where the "Opening Soon!" logic wasn't getting calculated and rendered properly. This PR fixes the issue.

Ensure the following interactions work as expected. Please test using a mobile form factor.

  • Tapping on a marker on the map displays information about the marker in a popup.
  • Tapping the "Show list of locations" button replaces the map view with a list view.
  • Tapping an item in the list replaces the list view with a map view, and navigates the map to the tapped item on the map.
  • Tapping one of the ✅'s in the legend filters items on the map and in the list.
  • Changing the language to spanish changes things on the page.
  • Clicking the Help/Info button opens and closes a menu with information.
  • Clicking the X Close button on the Help/Info screen closes the modal.

Check the app in the following web browsers:

  • Chrome
  • Firefox
  • Edge 😬
  • Safari

@jdalt
Copy link
Contributor

jdalt commented Jun 9, 2020

We recently added automated tests that run on TravisCI and they appear broken in this PR. They're new so it's possible that the tests themselves are at fault.

https://travis-ci.org/github/Twin-Cities-Mutual-Aid/twin-cities-aid-distribution-locations/builds/696291545
Build__6_-_Twin-Cities-Mutual-Aid_twin-cities-aid-distribution-locations_-_Travis_CI

It looks like the Mapbox token didn't make it into the test suite. I'm not certain why this would affect your PR but not others though.

I'll try and help out tomorrow, but if you want faster resolution you might want to get the attention of the folks who added the tests/

@alyiri
Copy link
Contributor Author

alyiri commented Jun 9, 2020

yeah, it looks like the secrets weren't configured yet in Travis but looking at other builds I think they are now. I'm not a member of the org, but I think if someone who is can rerun the build it should be okay. 🤞

@jdalt
Copy link
Contributor

jdalt commented Jun 10, 2020

@alyiri I'll add you to the org and I'll rerun the tests.

@jdalt
Copy link
Contributor

jdalt commented Jun 11, 2020

@alyiri I'm not able to re-run the tests yet (still sorting that out...) but if you could merge master, that would trigger another build. Apparently some stuff was missing in Travis. I'm hoping this will resolve itself after a merge.

@caseyhelbling
Copy link
Contributor

I just pulled this branch, ran the tests locally and they passed fine. I then reran the build on travis and they still fail. I think @jdalt has it right... let's get rebased/merged with master and then see if that fixes things. The settings in Travis for those env vars look to be setup correctly... so something is funky.

@jdalt jdalt added the Status: Needs Master Merge This Pull Request needs recent changes merged in. label Jun 13, 2020
@experimatt
Copy link
Contributor

@alyiri I also just added you to the GH org so that should make creating branches and PRs easier (in the future)

Copy link
Contributor

@omawhite omawhite left a comment

Choose a reason for hiding this comment

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

Code looks good to me, but i don't quite feel i have the authority to approve. Also haven't done any QA on this just fyi.

@@ -223,6 +223,16 @@ const getStatus = id => {
return status || unknownStatus
}

// calculates if current time is within one hour of opening time
Copy link
Contributor

Choose a reason for hiding this comment

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

what do you think about making this a jsdoc comment?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Needs Master Merge This Pull Request needs recent changes merged in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants