mohanadarafe / GuideMe

SOEN 390 | Winter 2020 | Mini Capstone
7 stars 5 forks source link

Search bar is now connected to BottomMenu and MoreDetails #213

Closed AlainJobs closed 4 years ago

AlainJobs commented 4 years ago

Please Test

Because I made some refactoring in important components (MapData.js), please make sure to test the app and try to break the Search and/or MoreDetails components, I believe in you all!

Bug fixed

Firassawan commented 4 years ago

Will review first thing in the morning! Thanks Alain!

mohanadarafe commented 4 years ago

After running npm i, run the following command:

expo r -c

In order to clear the cache for all the package additions

mohanadarafe commented 4 years ago

The MapData.js refactor is really good. Great documentation & makes the code look cleaner. I ran our Debt tracker on your file & it's all green ✅

I tried to break the MoreDetails/Search, I noticed this: IMG_8046

The addresses should not be in the search bar. Also, the code coverage sky rocketed down from 77% to 62%. I think it's due to route stack. @bkenza can help you get the coverage back up 📈

AlainJobs commented 4 years ago

Thanks @mohanad, I'll update the PR instructions and I will remove the addresses, it doesn't make sense to have them.

csbduzi commented 4 years ago

Amazing Job on the refactoring, a lot code was reduced in MapData and MoreDetails! Very clean! Concise documentation! ✅

I tested on my end by selecting items from the search bar and then pressing on the arrow up to go to MoreDetails and it works well. And its still smooth too.

Also I noticed on that certain MoreDetails screens that the number when left _empty" is not displaying N/A anymore.

Image from iOS (3)

csbduzi commented 4 years ago

The MapData.js refactor is really good. Great documentation & makes the code look cleaner. I ran our Debt tracker on your file & it's all green ✅

I tried to break the MoreDetails/Search, I noticed this: IMG_8046

The addresses should not be in the search bar. Also, the code coverage sky rocketed down from 77% to 62%. I think it's due to route stack. @bkenza can help you get the coverage back up 📈

image

He's right the code coverage has reduced significantly to 62%, however I don't think its because of the route stack since its doesn't have any methods.

On my end, not sure if it's locally, but the some tests are failing and maybe not covering the code properly

bkenza commented 4 years ago

The MapData.js refactor is really good. Great documentation & makes the code look cleaner. I ran our Debt tracker on your file & it's all green ✅ I tried to break the MoreDetails/Search, I noticed this: IMG_8046 The addresses should not be in the search bar. Also, the code coverage sky rocketed down from 77% to 62%. I think it's due to route stack. @bkenza can help you get the coverage back up 📈

image

He's right the code coverage has reduced significantly to 62%, however I don't think its because of the route stack since its doesn't have any methods.

On my end, not sure if it's locally, but the some tests are failing and maybe not covering the code properly

I will take care of the code coverage!

AlainJobs commented 4 years ago

@csbduzi For the N/A, it should work now.

bloulidi commented 4 years ago

I get this error when one of from/To is "Grey nuns Building" and i click on view routes. 92471387_291552458500978_4412167010658549760_n

AlainJobs commented 4 years ago

I get this error when one of from/To is "Grey nuns Building" and i click on view routes. 92471387_291552458500978_4412167010658549760_n

Thank you, I know why!

AlainJobs commented 4 years ago

Please guys, for any error, post those in the review section of Github in the appropriate file that broke, its easier to resolve and make the PR less hard to read.