Added store hours and store details #96

thumn · 2020-04-17T11:32:45Z

What's new in this PR

Store cards now display whether the store is currently open or what time it will next be open
The store card now navigates to a separate store card on click that provides information on the store's address, phone number, hours by day of the week, and accepted programs (NOTE: accepted programs is not finished)
Detailed store hour schedules bold the current day and hours

Relevant Links

N/A

Online sources

Date

Related PRs

N/A

How to review

Navigate through stores and make sure each store's card can successfully navigate to its corresponding store screen. The current day should be correctly bolded, and the store open status summary should correctly reflect whether the store is open or when it'll next be open. For example, Yang Market has a particular complicated store hours input.

Next steps

Start work on tags for accepted programs in the stores list. This will also be applied to the store screens.

NOTE: I'm getting a "each child in a list should have a unique 'key' prop" error -- does anyone know how to fix this? The file is StoreHours.js. I also can't figure out how to make the <StoreDetailText> primary green on the StoreCard, but it works fine on the store screen. (Might be passing in the wrong prop, but the component seems to take in color as a prop so I'm not too sure.)

Also, I sometimes get a weird "store is undefined" error on MapScreen that goes away if I reload the app. Sadly this might end up circling back to the useFocusEffect hook issue/refactoring task we set aside :(

Tests Performed, Edge Cases

Tested with hard-coded store hour inputs:
9am-8pm Tu-Sat, 9am-7pm Sun, Closed Mon
8am-7pm Daily
Open 24/7
7am-Midnight Daily

Also manually compared hours between Stores table in the Airtable and the ones that showed up in the store screens. I looked up the more edge-case-y stores and compared the info on the store card and corresponding store screen. (e.g. Yang Market, Mellon Market, T & G Grocery, Smiley Mart).

Screenshots

CC: @tommypoa @wangannie

… card for some reason

thumn · 2020-04-17T11:38:28Z

LOL Just remembered I'm using the wrong-facing phone icon...

wangannie

THIS IS SO GREAT! The comments you left are SUPER helpful for documentation and helping me figure out how it all works. I really appreciate that you took the time to add those!

The Open Now isn't working for me right now...I noted why I think this might be happening but it's very strange. Besides that, I noted a few ways we can maybe accommodate some different input styles/edge cases (better for handoff) + cleanup. Great work Thu 😍 !

wangannie · 2020-04-17T16:39:14Z

screens/map/MapScreen.js

@@ -197,7 +191,13 @@ export default class MapScreen extends React.Component {

  renderContent = () => {
    return (
-      <BottomSheetContainer>
+      <TouchableOpacity


Right now the whole map drawer becomes the button -- probably add this touchable opacity to StoreCard instead (so it also works from StoreListScreen.

Also, if TouchableOpacity continues to do this, it might be better to switch to TouchableHighlight or something that doesn't do this when clicked 😂

wangannie · 2020-04-17T16:55:20Z

lib/mapUtils.js

+  }
+
+  const convertedOpeningTime = openingHour * 60 + openingMin;
+  const convertedClosingTime = closingHour * 60 + closingMin;


This is always showing up like it's currently closed for every store.

When I logged the current date & opening & closing times, I got this so I think it's concatenating openingHour*60 with openingMin instead of adding 😂 and same with closing. Wild.

When I added parseInt for min, it works well!

Oh wait also it only seems to work for Nam's (the first store that loaded for me bc of the simulator location I set). When I select any other store, it says the convertedOpening and convertedClosing are NaN, even though the todaysStoreHours is correct. Very odd..

Ah shoot I forgot the minutes were strings RIP thanks for catching that

Ahh also the issue was that some of the times don't have minutes so I didn't check for minutes before converting something that didn't exist! So that's why it's NaN but that's fixed now tyty

wangannie · 2020-04-17T16:57:34Z

navigation/stack_navigators/StoresStack.js

@@ -50,6 +51,7 @@ export default function StoresStackNavigator() {
          },
        }}
      />
+      <StoresStack.Screen name="StoreScreen" component={StoreScreen} />


Hm could you rename this to StoreDetailsScreen for consistency with ProductDetailsScreen? StoreScreen could maybe get confused with StoreListScreen.

wangannie · 2020-04-17T16:58:44Z

screens/map/StoreScreen.js

+          </NavButton>
+          <NavTitle>{store.storeName}</NavTitle>
+        </NavHeaderContainer>
+        <View style={{ marginLeft: 16, marginTop: 30 }}>


Can you make this a ScrollView? The size is fine not scrolling on a larger phone, but it would get cut off on smaller devices.

wangannie · 2020-04-17T17:17:04Z

components/store/StoreCard.js

-        <StoreDetailText>{storeHours}</StoreDetailText>
+        <StoreDetailText
+          color={
+            storeOpenStatus === 'Open 24/7' ? Colors.primaryGreen : Colors.black


I think I was also having this issue with the 'Earn and redeem Healthy Rewards here' line, so I added a prop greenText to StoreDetailText (see line 114), so you can probably just move this logic into something like that.

And probably change to a .includes('Open')

wangannie · 2020-04-17T17:59:02Z

lib/mapUtils.js

+  const closingMin = closingHourMinSplit[1];
+
+  // Convert to 24 hour time
+  if (openingSuffix === 'pm') {


General comment: We'll definitely give the NPO a strict guide on how they have to input hours data into Airtable, but when possible, we should try to leave a bit of room for error. As you noticed when pulling the phone numbers from the original sheet, the data was quite messy and inconsistent and I had to clean everything up first.

In places like this, could you try to make things like this not case sensitive (like maybe add a toLowerCase toopeningSuffix in case they try to enter things with uppercase PM)? Maybe same for the abbreviations in daysOfTheWeek.

Also, the original spacing format they gave us was very inconsistent (some with spaces before hyphens, some with spaces after, some both/neither etc). To correct for this, could you add something like hours.replace('- ', '-').replace(' -', '-') to get rid of spaces around hyphens in both hours and days before you even try to parse it to make sure it doesn't break? (also there might be a cleaner way to do this than replace)

I know these seem like small details, but if it takes a few small changes to avoid NPO user error causing crashes, let's try to make their experiences easier whenever we can : )

In the airtable it looked like the hours were inputted in the form of:
7:30am-10pm Daily
7am-10pm Mon-Sat, 9am-9pm Sun

Would it be safe for me to assume that it would follow the timeInterval Day format (regardless of weird spacing issues)? I haven't accounted for the ones you linked in the picture

wangannie · 2020-04-17T18:01:28Z

screens/map/StoreScreen.js

+          </TouchableOpacity>
+          {/* Phone Number */}
+          <InLineContainer style={{ alignItems: 'center', paddingBottom: 32 }}>
+            <FontAwesome5 name="phone" solid size={24} color={Colors.black} />


Change all instances of Colors.black to activeText

wangannie · 2020-04-17T18:07:04Z

components/store/StoreHours.js

+    return (
+      <View style={{ display: 'flex', flexDirection: 'row' }}>
+        {/* Days of the week */}
+        <View style={{ width: '50%' }}>


Nit: Maybe adjust this 50-50 ratio to more 40-60 just to make sure we don't get weird wrapping/truncating edge cases on narrower screens.

wangannie · 2020-04-17T18:08:19Z

components/store/StoreHours.js

+        </View>
+        <View style={{ width: '50%' }}>
+          {daysOfTheWeekFull.map((day, _) => {
+            if (day == todaysDay) {


I think we should make sure all of these are === instead of == bc eslint haha

wangannie · 2020-04-17T18:27:33Z

lib/mapUtils.js

@@ -43,3 +43,156 @@ export async function getProductData(store) {
 export const getMaxWidth = function sync(screenWidth, ebt, seeProducts) {
  return screenWidth - 32 - 12 + (ebt ? -48 : 0) + (seeProducts ? -130 : 0);
 };
+
+export function constructHoursDict(hours) {
+  const daysOfTheWeek = {


Wondering if we should consolidate some of these things into a file in constants? Lots of repeated code between these different functions.

…d for those that are not provided

wangannie

SO SORRY here are some more fun edge cases :')

wangannie · 2020-04-18T00:47:22Z

components/store/StoreHours.js

+
+  const parseHours = () => {
+    // Case: hours = "Open 24/7"
+    if (hours === 'Open 24/7' || hours === 'Store hours unavailable') {


Hm then I'm not sure why it isn't working? When I make a field in Airtable blank and tap on that store on the map, it breaks.

I think it's bc parseHours isn't called when trying to find if the store is currently open. You'd also need to check when computeStoreOpen is called if hours is blank or somehow invalid. I would suggest maybe puttting a try/catch around the whole computeStoreOpen for any parsing/date issues, and returning 'Store hours unavailable' in the catch.

wangannie · 2020-04-18T01:03:16Z

lib/mapUtils.js

+  let tmrwDay = daysOfTheWeekFull[tmrwDayIndex];
+  let tmrwStoreHours = hoursPerDayDict[tmrwDay];
+  // Loop until next open day is found.
+  while (tmrwStoreHours === 'Closed') {


NICE that you catch this! I was expecting this edge case 9am-7pm Sun, 9am-8pm Tu-Thurs, Closed Fri, Closed Sat to break but that's very cool that it works! Still, I'd probably rename this to be more specific like next opening day bc 'tmrw' is a bit deceiving.

wangannie · 2020-04-18T01:22:53Z

components/store/StoreCard.js

@@ -55,89 +64,108 @@ export default function StoreCard({
    });
  };

+  const storeOpenStatus = computeStoreOpen(store.storeHours);


I'm getting REALLY SLOW loading! Is there a way we can make this only call once? It seems to be called with every movement on the map and gets REALLY REALLY slow on the store list screen. When I type to search, it's so slow that it doesn't even respond at all. It gets to a point where I can't load all the stores on storelist and then it fully stops responding altogether like I can't tap map icons or close the Find a Store page.

Yeah good point! I removed one call in the StoreDetailsScreen and pass the summary line in as a prop from StoreCard. Hopefully this helps!

wangannie · 2020-04-18T01:24:37Z

components/store/StoreCard.js

-          {`${distance} miles away`}
-        </Caption>
-      )}
+    <TouchableHighlight


Ok sorry this totally my bad this was a bad suggestion. This turns black on clicking 😅 TouchableOpacity would work just fine instead since it's on StoreCard and not transparent to the whole mapscreen now 😂

wangannie · 2020-04-18T04:06:41Z

lib/mapUtils.js

@@ -155,7 +152,8 @@ function computeNextOpenDay(todaysDayIndex, hoursPerDayDict) {
  return 'Closed until '
    .concat(tmrwDay)
    .concat(' ')
-    .concat(tmrwOpeningTime);
+    .concat(tmrwOpeningTime)
+    .replace('12am', 'Midnight');


I'm p sure you actually wouldn't need this here or in 'Open now until' below bc doing this in the final StoreHours does the final change.

Oh I added it there because I don't display StoreHours on the StoreCard (this method primarily gives you the summary, aka "Open 24/7" or whatever, and StoreHours gives you the whole schedule)

wangannie

Small details I forgot to submit

screens/map/StoreDetailsScreen.js

wangannie · 2020-04-18T05:38:29Z

lib/mapUtils.js

+  if (hoursStrSplit.length === 2 && hoursStrSplit.includes('Daily')) {
+    const time = hoursStrSplit[0];
+
+    daysOfTheWeekFull.map((day, _) => {


WRT the 'Each child in an array should have a unique "key" prop.' issue, could you set key={day} since that should (hopefully) always be unique from the dictionary? Or you could do this like renderProductList in ProductsScreen but eslint will yell at you for Do not use Array index in keys

return products.map((product, i) => { return ( <ProductCard key={i} product={product} navigation={this.props.navigation} store={store} displayPoints /> ); });

I added keys to all the component children of the loop in StoreHours and it seems to get rid of the warning so we good!

lib/mapUtils.js

wangannie

GREAT WORK THU thank you for all of your time and energy on this!

…chen into thu/hours

tommypoa

Oh damn this is such a great PR Thu! Really helpful documentation and very clean/nice code to read! Left literally 2 nits/question that I don't think it's crucial to address in this PR but probably nice – otherwise good to go! Awesome!

tommypoa · 2020-04-18T17:52:43Z

screens/map/StoreDetailsScreen.js

+          <TouchableOpacity style={{ paddingBottom: 32 }}>
+            <InLineContainer style={{ alignItems: 'center' }}>
+              <FontAwesome5
+                name="directions"
+                size={24}
+                color={Colors.activeText}
+              />
+              <Body style={{ marginLeft: 12 }}>{store.address}</Body>
+            </InLineContainer>
+          </TouchableOpacity>


Is there a functionality that is associated with this TouchableOpacity? Best guess is that it should mimic that of what happens when you do it for the BottomSheet, but it's doing nothing for now.

tommypoa · 2020-04-18T17:57:51Z

components/store/StoreHours.js

+      return hoursList(hoursPerDayDict);
+    }
+    console.log(
+      '[Storehours] parseHours: Issue parsing store hours. Hours were: ',


Nit: StoreHours

annieyro

amazing @thumn !! I'm not sure on the React details but if things are still kind of slow, I would guess that you don't want StoreHours to be a functional component, and instead have it maintain its state so it doesn't re-parse/construct the dictionary every time.

Another alternative is to parse the hours in lib/mapUtils's updateStoreData function, and set a new property that is an enum telling you what category of 'hours' to show the parsed hours is (seems like it's something like 24/7, daily, or other?). Then you can pass the flag / enum property, check that, and then manipulate the store.hours property as needed (at that point it could be a string or a dictionary).

Also, a note for the future @wangannie : for deciding whether to extract constants/etc to files, it’s usually fine to keep it in the lib/_utils file unless the constants are used independent of the lib/_utils; otherwise it’s just always gonna be two imports: one from the constants/file and one from the lib/_utils file.

Another reason is that constants are hardcoded values that might be updated (e.g rewardPointValue)

From what I can tell it wasn't strictly necessary to bring the days of the week out but it's fine either way LOL

wangannie · 2020-04-18T20:01:56Z

Getting this in the StoreList sometimes

VirtualizedList: You have a large list that is slow to update - make sure your renderItem function renders components that follow React performance best practices like PureComponent, shouldComponentUpdate, etc. Object {
  "contentLength": 4548,
  "dt": 754,
  "prevDt": 662,
}

thumn added 7 commits April 15, 2020 22:37

started work on parsing algorithm

b358740

progress on store hours parsing

e56c4e3

parsing algo is finished

84de47f

store screen

ea2c57a

finished computeStoreOpen

1def44c

finished working on store hours

270c8be

fixed open 247 green color, but the color doesnt show up on the store…

fc67784

… card for some reason

thumn requested review from wangannie and tommypoa April 17, 2020 11:32

wangannie requested changes Apr 17, 2020

View reviewed changes

thumn added 2 commits April 17, 2020 15:59

responded to comments from six

71de190

more edge case handling, specifically filling in store hours as Close…

1f50305

…d for those that are not provided

wangannie requested changes Apr 18, 2020

View reviewed changes

replace 12am with Midnight

8849ccd

wangannie reviewed Apr 18, 2020

View reviewed changes

more bug squashing thanks six

204cdd3

wangannie reviewed Apr 18, 2020

View reviewed changes

thumn added 3 commits April 18, 2020 00:25

some fixes for bad loading

033259c

fixed store hours status rendering by changing map to for loop

7f06c49

fixed key warning, scrollview

e2599eb

wangannie reviewed Apr 18, 2020

View reviewed changes

lib/mapUtils.js Outdated Show resolved Hide resolved

kwfk reviewed Apr 18, 2020

View reviewed changes

lib/mapUtils.js Outdated Show resolved Hide resolved

kwfk reviewed Apr 18, 2020

View reviewed changes

lib/mapUtils.js Outdated Show resolved Hide resolved

This was referenced Apr 18, 2020

Added accepted programs tags #98

Merged

External linking for address and phone number, store card info icon, and misc products screen styling #99

Merged

string concat fixes, removing an lol

2c1bfde

wangannie approved these changes Apr 18, 2020

View reviewed changes

Merge branch 'master' of https://github.com/calblueprint/dccentralkit…

8890bc9

…chen into thu/hours

tommypoa approved these changes Apr 18, 2020

View reviewed changes

wangannie merged commit 27c7777 into master Apr 18, 2020

wangannie deleted the thu/hours branch April 18, 2020 19:47

annieyro reviewed Apr 18, 2020

View reviewed changes

thumn mentioned this pull request Apr 30, 2020

Fixed edge cases with store hours #141

Merged

Added store hours and store details #96

Added store hours and store details #96

Conversation

thumn commented Apr 17, 2020

What's new in this PR

Relevant Links

Online sources

Related PRs

How to review

Next steps

Tests Performed, Edge Cases

Screenshots

thumn commented Apr 17, 2020

wangannie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wangannie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wangannie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wangannie left a comment

Choose a reason for hiding this comment

tommypoa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

annieyro left a comment

Choose a reason for hiding this comment

wangannie commented Apr 18, 2020