Consolidation of bugs/Features & Redux implementation

AlainJobs commented 4 years ago

Overview

This Pull Request was, in the beginning, focused on finding a solution for when the app kept being disconnected due to a memory leak caused by infinite intervals of AsyncStorage calls. This led me to make some more research into what could be done. The goal was also to avoid Redux since it could take a while. However, while attempting to use Redux, I made it work, thus I thought it could be a nice addition. It made the app quite faster even with the number of new components/screens added. Disclaimer: I didn't touch anything in the indoor components & SideMenu. There are still some warning this and there (not supposded to have in the files I've changed.)

Additions

Map is updated when clicking an item + marker in searchBar

ezgif com-optimize

Polygons color and stroke width are added and dynamic + unselect

ezgif com-video-to-gif (1)

Toggle Campus is now dynamic and will move the map

ezgif com-video-to-gif (2)

Click on map to deselect anything (Even searchBar context)

ezgif com-video-to-gif

⌚ 📈 App should be faster

The only components for which redux were not implemented are the SideMenu screens and Indoor.

Test the branch

Checkout to feature/redux-implementation
Do npm install
If step 2 still create crashes in the build: Try the following
- For Mac: rm -rf node_modules and redo npm install.
- For Windows: open file explorer of the project and delete the node_modules folder and redo npm install.
Do npm start or expo start
test the new features and try to break them.

Small Note

PreviewDirections and Directions arrow back buttons may be hard to click now, @csbduzi will take of it. 🥇

Missing

Before you add comments on unit testing, I am aware.

Resolved Issue/Bug/Feature

Resolves #227 Resolves #221 Resolves #149 Resolves #147

bloulidi commented 4 years ago

The marker is not placed in the right building when i choose EV building.

csbduzi commented 4 years ago

Any particular reason why Redux was not implemented for indoor and side menu?

csbduzi commented 4 years ago

A user might be tempted to press on the initials of a building, maybe try to have an onPress on the markers (characters) also, else they might think pressing on a building is not as responsive

csbduzi commented 4 years ago

The marker is not placed in the right building when i choose EV building.

Yes, the coordinates for the EV building would have to be changed in the buildingData file

csbduzi commented 4 years ago

the onPress on the map to make the drop down list disappear is not as responsive 😥

It works a times or sometime I feel like I have to press on specific spots on the map

bloulidi commented 4 years ago

Great job alain !! This is the cherry on top of the cake. 🍰 It feels very fluid and i love the new features. It makes the selection of the buildings a lot more responsive than before. MASHALLAH !!!

AlainJobs commented 4 years ago

Any particular reason why Redux was not implemented for indoor and side menu?

Not really, you guys were working on it for a while and ultimately, you are not using asyncStorage calls in setIntervals as heavily as in BottomMenu and other components. I don't think it's necessary and we don't really have the time...

AlainJobs commented 4 years ago

A user might be tempted to press on the initials of a building, maybe try to have an onPress on the markers (characters) also, else they might think pressing on a building is not as responsive That would be another thing. It would just be a matter of adding the logic inside BuildingIdentification components like we did for BuildingHighlights. That would be nice if we have time

AlainJobs commented 4 years ago

the onPress on the map to make the drop down list disappear is not a responsive 😥

It works a times or sometime I feel like I have to press on specific spots on the map

It's legit the best we can do for the onPress event... Any other solution would have to use another event.

AlainJobs commented 4 years ago

I have no comments regarding the code. I tested the app & could not reproduce a bug (other than the marker mentioned above). Redux changes the game & the app is so much more smoother than have 30+ simultaneous calls every 1ms. Incredibly well done to resolve other issues & add little features here & there that enhance the user experience.

I will work on organizing unit testing after your PR is merged. For now, approved for me ✅

We move! 🤝

AlainJobs commented 4 years ago

The marker is not placed in the right building when i choose EV building.

Resolved!

mohanadarafe commented 4 years ago

I found a small bug. When searching Men's/Women's Washroom or Water Fountain (Indoor POI), the app responds with:

null is not an object (evaluating 'searchItem.coordinates.latitude')

AlainJobs commented 4 years ago

I found a small bug. When searching Men's/Women's Washroom or Water Fountain (Indoor POI), the app responds with:

null is not an object (evaluating 'searchItem.coordinates.latitude')

It makes sense, it should NOT be available in the mainSearchBar as I suggested last time.

csbduzi commented 4 years ago

I found a small bug. When searching Men's/Women's Washroom or Water Fountain (Indoor POI), the app responds with:

null is not an object (evaluating 'searchItem.coordinates.latitude')

It could be added to the DoubleSearch search bars manually instead, just like we did with current location and point of interest, and removed as soon as we pressed on the back button.

AlainJobs commented 4 years ago

As soon the washrooom and the waterfountain bug will be fixed, I will approve the PR

It's fixed, but I'm doing more stuff. And I have Kenza changes to fix.

AlainJobs commented 4 years ago

Update

The currentBuildingLocation is implemented. I made some changes to it. It will update the bottom menu and highlight in blue the building your in. @mohanadarafe is adding the CurrentLocationButton on Map.js

mohanadarafe / GuideMe