mohanadarafe / GuideMe

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

Feature/preferences: Add DoubleSearch screen, PreferenceMenu page, layout of the Directions screen. #134

Closed bkenza closed 4 years ago

bkenza commented 4 years ago

Pull request contents:

In this PR, @csbduzi and I have worked on creating a DoubleSearch screen, which is a screen that contains two searchbars (one for the starting point and one for the destination point). This screen is linked to two paths so two scenarios can happen:

  1. User is on the main screen, the map, they click on a building and want to see more details. Upon viewing and scrolling through the MoreDetails page, they decide to click on the Get Directions button, which takes them to the screen with the two searchbars. The user can customize their location points or simply click on the View Route button. A back button has also been implemented to bring the user back to the previous screen.

  2. User is on the main screen, the map, and they click on the searchbar to look for a specific building. As soon as an item is selected from the search bar's dropdown, the bottom menu changes to show the name of the building selected, and a Get Directions button is shown as well. If the user chooses to click on this button, they are taken to the DoubleSearch screen, where they can either customize their location points first, or just view the route by clicking on the View Route button. A back button has also been implemented to bring the user back to the MoreDetails page.

Furthermore, we have created a PreferenceMenu screen, which is an extension of the BottomMenu that becomes available for use right before a user can start viewing the step by step directions. In this screen, the user can select the type of user they are (graduate student, undergraduate student, visitor or university staff), wether or not they have reduced mobility, and finally their preferred mode of travel (driving, walking, bus or the Concordia shuttle).

Finally, a layout of the screen that will contain a preview of the directions in the future has been set up, but it should be noted that it is not currently functional.

It is worth mentioning that this PR fixes the following issues: #18 #25 #26 #27 #28 #118 #125

How to test:

  1. Run the app using the command expo start

Scenario 1:

  1. On the main screen, select a building and then click on the up arrow located on the bottom menu to view more details.
  2. Scroll all the way down and click on Get Directions.
  3. You are now on the DoubleSearch screen and you can customize your location points.
  4. Click on view Route.
  5. Click on the back button to go back to the DoubleSearch screen.
  6. Click on the back button to go back to the MoreDetails page.

Scenario 2:

  1. On the main screen, start typing on the search bar and select an item from the drop down list.
  2. Click on Get Directions located on the bottom menu.
  3. You are now DoubleSearch screen and you can customize your location points.
  4. Click on view Route.
  5. Click on the back button to go back to the DoubleSearch screen.
  6. Click on the back button to go back to the main screen.
mohanadarafe commented 4 years ago

Good job guys, I really like the animations! The front-end is nice & slick and there is no problems with the BottomMenu which was tough to manage I'm sure. However, I detected a bug & FE issue.

Bug

1) Load up app 2) Tap on any building 3) Map refreshes back to initial position ezgif-7-012f6488a494

FE Issue

Minor one, I am on my iPhone 10 & the Get Directions button seems to be too big! ![Uploading IMG_7732.PNG…]()

AlainJobs commented 4 years ago

Reason of Bug

Following bug reported by @mohanadarafe . I tested it and the issue occurs whenever you click on the map, i.e the onPress event (screenshot). When commented out, the bug disappears.

Screenshot

image

Issue Introduced

However, after discussion with the authors, that line was added to make the search context collapste when clicking on the map (which is useful).

csbduzi commented 4 years ago

Reason of Bug

Following bug reported by @mohanadarafe . I tested it and the issue occurs whenever you click on the map, i.e the onPress event (screenshot). When commented out, the bug disappears.

Screenshot

image

Issue Introduced

However, after discussion with the authors, that line was added to make the search context collapste when clicking on the map (which is useful).

The fact that this line makes the app bug, an alternative path would have to be found to keep track of when the user presses on the screen for the collapsing the drop down list

bkenza commented 4 years ago

Overall

Good, but will need refactoring!

Not so good

We should stop running the eslint that automatically change the code syntax to make it consistent with the good practise as this biases the commits. We have 33 files changed but only few files were actually meaningful for this PR. In the future, we should only run eslint on master after a PR.

I agree! We should run eslint on master after every PR is pushed. An alternative would be to set up the eslint fix on save (in the vs code settings). That way, all modified files will be fixed without touching any other files.

bkenza commented 4 years ago

PR was updated as per the comments above :)

mohanadarafe commented 4 years ago

Overall

Good, but will need refactoring!

Not so good

We should stop running the eslint that automatically change the code syntax to make it consistent with the good practise as this biases the commits. We have 33 files changed but only few files were actually meaningful for this PR. In the future, we should only run eslint on master after a PR.

We should run npm run lint in travis ci script. There are 16 errors right now on lint.

bkenza commented 4 years ago

We should run npm run lint in travis ci script. There are 16 errors right now on lint.

We have agreed to run it on master, since a bunch of files that have nothing to do with the PR get modified.