mohanadarafe / GuideMe

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

Indoor and Outdoor are merged. #202

Closed AlainJobs closed 4 years ago

AlainJobs commented 4 years ago

Note

Please test the branch locally, some late commits were made but were not fully tested on our end.

Description

We have 4 scenarios.

  1. If origin and destination are classrooms from the same building --> IndoorMap Only
  2. If the origin is a classroom but not the destination. --> Mixed Indoor-Outdoor
  3. If the destination is a classroom but not the origin. --> Mixed Indoor-Outdoor
  4. If both origin and destination are classrooms but are from different buildings --> Mixed Indoor-Outdoor
    • Concerning 4), we added a building in Loyola Campus.

Some UI changes were made as well:

Coders

@mohanadarafe and I both contributed to this feature.

This fixed task #197

bloulidi commented 4 years ago

When my one of the search fields is Hall building and the other one is a classroom inside Hall building. The application crashes with the following message. 91907864_1667530163401098_984568420215291904_n

bloulidi commented 4 years ago

When i am inside the double search bar screen and i want to change the text inside the fields, I felt that some things were not great for the UX. First, when i click on the text displayed inside the search bar it does not disapear. I tried to remove it with "backspace" but it didn't want to. Because of that, i though that the text was still inside and i couldn't remove it. However, i realized that if i start writing, the new text started appearing. 92204036_1099302913757521_6102055707547271168_n

bloulidi commented 4 years ago

When one of the fields is John molson executive center, it gives the following error. 92280063_245069093284386_4974579917074202624_n

bloulidi commented 4 years ago

Other than that, i found that the mixing of indoor/outdoor was implemented well especially with the short amount of time we have. Great job boys !! @AlainJobs @mohanadarafe 🚀🏆

AlainJobs commented 4 years ago

Testing the branch:

I ran the app on your branch and tested this feature using: From: "VL103" To: "H801" The code works as expected! You guys have done a great job merging indoor and outdoor directions. Adding a building icon was a nice touch and it makes the transition between outdoor-indoor and vice-versa more intuitive for the user.

What could be improved:

When entering a building the user has to manually click on the floor and that's not super intuitive. Edit: I tested with the same parameters as @badred123 and i get the same error as well: image

Overall:

I know that merging indoor and outdoor directions wasn't the easiest thing but you guys have done and awesome job like always! @AlainJobs & @mohanadarafe

for what Could be improved, it is on the tasks to do in the PR description, but yeah thank you! 🤖

AlainJobs commented 4 years ago

When i am inside the double search bar screen and i want to change the text inside the fields, I felt that some things were not great for the UX. First, when i click on the text displayed inside the search bar it does not disapear. I tried to remove it with "backspace" but it didn't want to. Because of that, i though that the text was still inside and i couldn't remove it. However, i realized that if i start writing, the new text started appearing. 92204036_1099302913757521_6102055707547271168_n

I perfectly understand your concern, but this is on master and it's not something we introduced. The reason of this, is that the placeholder color is black. I'm also against it because of what you described. It should be grey or if we want to keep it black, that search field should be not changeable. 🚀

AlainJobs commented 4 years ago

When my one of the search fields is Hall building and the other one is a classroom inside Hall building. The application crashes with the following message. 91907864_1667530163401098_984568420215291904_n

I will add an error handling for that case, thank you. However, why would you go from Hall Building to a classroom, instead of CurrentLocation to a classRoom?

bloulidi commented 4 years ago

When i am inside the double search bar screen and i want to change the text inside the fields, I felt that some things were not great for the UX. First, when i click on the text displayed inside the search bar it does not disapear. I tried to remove it with "backspace" but it didn't want to. Because of that, i though that the text was still inside and i couldn't remove it. However, i realized that if i start writing, the new text started appearing. 92204036_1099302913757521_6102055707547271168_n

I perfectly understand your concern, but this is on master and it's not something we introduced. The reason of this, is that the placeholder color is black. I'm also against it because of what you described. It should be grey or if we want to keep it black, that search field should be not changeable. 🚀

Okay i will create an issue regarding that if its not already done.

AlainJobs commented 4 years ago

When one of the fields is John molson executive center, it gives the following error. 92280063_245069093284386_4974579917074202624_n

That's because that is not a building, its a service or department. The DoubleSearch bar should not display services and departments. It's also on the next things todo! I will add an error handling!

bloulidi commented 4 years ago

When my one of the search fields is Hall building and the other one is a classroom inside Hall building. The application crashes with the following message. 91907864_1667530163401098_984568420215291904_n

I will add an error handling for that case, thank you. However, why would you go from Hall Building to a classroom, instead of CurrentLocation to a classRoom?

Okay! You normally would not, but the application allows it. We just have to make sure it does not make it crash.

bloulidi commented 4 years ago

When one of the fields is John molson executive center, it gives the following error. 92280063_245069093284386_4974579917074202624_n

That's because that is not a building, its a service or department. The DoubleSearch bar should not display services and departments. It's also on the next things todo! I will add an error handling!

I see ! I will create an issue for that also.

bloulidi commented 4 years ago

When i am inside the double search bar screen and i want to change the text inside the fields, I felt that some things were not great for the UX. First, when i click on the text displayed inside the search bar it does not disapear. I tried to remove it with "backspace" but it didn't want to. Because of that, i though that the text was still inside and i couldn't remove it. However, i realized that if i start writing, the new text started appearing. 92204036_1099302913757521_6102055707547271168_n

I perfectly understand your concern, but this is on master and it's not something we introduced. The reason of this, is that the placeholder color is black. I'm also against it because of what you described. It should be grey or if we want to keep it black, that search field should be not changeable. 🚀

Okay i will create an issue regarding that if its not already done.

https://github.com/mohanadarafe/GuideMe/issues/204

bloulidi commented 4 years ago

When one of the fields is John molson executive center, it gives the following error. 92280063_245069093284386_4974579917074202624_n

That's because that is not a building, its a service or department. The DoubleSearch bar should not display services and departments. It's also on the next things todo! I will add an error handling!

I see ! I will create an issue for that also.

https://github.com/mohanadarafe/GuideMe/issues/203

AlainJobs commented 4 years ago

Update on regex expression and map function

I thought it was on my end (OutDoor) because a similar regex, but its an indoor issue. Also, I think you guys just added a scenario. A classroom to the building exit or the opposite, the building entrance to the classroom. The indoorMap view is already implemented but the logic must be extended. In fact, I realized that the Double search Bar should have indoor point of interest! Thanks Guys 🥇, an issue/task should be created for this.

bloulidi commented 4 years ago

Update on regex expression and map function

I thought it was on my end (OutDoor) because a similar regex, but its an indoor issue. Also, I think you guys just added a scenario. A classroom to the building exit or the opposite, the building entrance to the classroom. The indoorMap view is already implemented but the logic must be extended. In fact, I realized that the Double search Bar should have indoor point of interest! Thanks Guys 🥇, an issue/task should be created for this.

I am not sure to understand the issue. 😅

AlainJobs commented 4 years ago

Update on regex expression and map function

I thought it was on my end (OutDoor) because a similar regex, but its an indoor issue. Also, I think you guys just added a scenario. A classroom to the building exit or the opposite, the building entrance to the classroom. The indoorMap view is already implemented but the logic must be extended. In fact, I realized that the Double search Bar should have indoor point of interest! Thanks Guys 🥇, an issue/task should be created for this.

I am not sure to understand the issue. 😅

AHAHAHAH! It's okay. What I'm saying is that, instead of having classroom to the same building like you did, it should be classroom to exit

csbduzi commented 4 years ago

When i am inside the double search bar screen and i want to change the text inside the fields, I felt that some things were not great for the UX. First, when i click on the text displayed inside the search bar it does not disapear. I tried to remove it with "backspace" but it didn't want to. Because of that, i though that the text was still inside and i couldn't remove it. However, i realized that if i start writing, the new text started appearing. 92204036_1099302913757521_6102055707547271168_n

The reason why the text does not disappear is because the value was set as the placeholder text and not as a "default value", this was a temporary fix, @AlainJobs , @bkenza and I tried to find a fix couple nights ago, however its still an open issue

bkenza commented 4 years ago

Tested again after the changes and everything seems to be working! Great job @AlainJobs & @mohanadarafe :)