Bite Selection UI revamp

[atharva-kashyap]

Relevant GitHub Issue: #17

Please navigate to the following routes after starting the app (Each route corresponds to each mockup):

• test_bite-selection-ui/button_overlay_selection: User is allowed to click on buttons that are overlayed on top of the image
• /test_bite-selection-ui/point_mask_selection: User has the ability to select a point on the image and is provided with three mask options around that point, out of which the user gets to choose one
• /test_bite-selection-ui/food_name_selection: User is allowed to choose a food item based on its name
  You don't have to manually navigate to these routes. You only need to navigate to one of them and once you explore that page, you may simply press the "Next" button below and it will navigate to the next route.

Explain how this pull request was tested, including but not limited to the below checkmarks.

All possible user interactions from the pages developed have been tested. None of the navigation buttons at the top (Home/Video/etc.) have been disabled. However, the "Locate Plate" and "Done Eating" buttons have been disabled.

---

Before creating a pull request

• Format React code with npm run format
• [N/A] Format Python code by running python3 -m black . in the top-level of this repository
• Thoroughly test your code's functionality, including unintended uses.
• Thoroughly test your code's responsiveness by rendering it on different devices, browsers, etc.
• Consider the user flow between states that this feature introduces, consider different situations that might occur for the user, and ensure that there is no way for the user to get stuck in a loop.

Before merging a pull request

• Squash all your commits into one (or Squash and Merge)

[amalnanavati]

Overall this looks good and is nice and functional! Good work :)

Many of my in-line comments are ++ things that don't need to be addressed right now. I'd suggest you move forward with the following steps:

1. Add the Router and revert main app functionality back to what it was.
2. Address eslint and prettier issues. Given how many have accumulated, I think it would be easier if you address them now before moving onto the other components. That way, you'll know how to write your code for those components in a way that avoids linting errors.
3. Implement the Name Display component.
4. Add the ability to move between these "mock-ups" upon button press. As you suggested in the meeting, I would fully use the router, e.g., localhost:3000/bite_selection_mockups/all_foods_segmented so you don't need to modify global state.
5. Add the mask display mock-up.
6. Implement the ++ things I mentioned in the in-line comments.

[amalnanavati]

Not crucial since these are mock-ups, but FYI I have been reading more into React Hooks and I think the best practice is to use useCallback for all functions that we don't want to re-define every re-render. So ideally this and all the below functions should be within useCallback e.g.,

const locatePlateClicked = useCallback(() => {
    console.log('doneEatingClicked')
    setMealState(MEAL_STATE.R_StowingArm)
}, [setMealState])

[atharva-kashyap]

Thanks! I will read more into this! For the sake of this mockup, I will keep it as is but if any of these components get through to the next stage, I will make sure to change it to reflect this new format of React Hooks.

[amalnanavati]

Functionality-wise both "Button Overlay" and "Buttons with Names" look good.

[amalnanavati]

The combination of underscores and hyphens is confusing -- stick to one.

[atharva-kashyap]

This is intentional! I assume "test" is to indicate that this route is for testing and anything that comes after is just a descriptive name for the particular case. In your case, where you already have the ros route, this issue doesn't arise because it is just a single word. But, I feel that it might be better to have the description and name separated. Let me know if you have strong thoughts against this.

[amalnanavati]

Hmm I get what you're saying, but we can still have "test" be the first word regardless of whether it is followed by a hyphen or underscore, and that would still indicate that this route is for testing, and anything that comes after it can still be the descriptive name (regardless of whether it has hyphens or underscores).

So I still think everything should be consistent. Either change test_ros to test-ros and make these routes test-bite-selection-ui... or make these routes test_bite_selection_ui -- I'm fine with either option.

[amalnanavati]

Tested it and functionality looks good! Good work :)

I am approving (all the changes I requested in the comments are small), but before merging you should do the following:

1. Merge main in, resolve any merge conflicts.
2. Address PR comments. For the comments that are not for this "mock-up" PR but should be addressed before implementing the final version, please leave them unresolved so we can quickly look back at those when we are implementing the final version.
3. Thoroughly test it (since the code may have changed slightly after merging main in).
4. Ensure the checks all pass. (Note that the checks have not run so far on your branch due to the merge conflict, they should run after you merge main in)
5. Squash and Merge this PR into main.

Good work :)

[amalnanavati]

No need to do this in this PR, but you'll see that the new main now has a folder src/buttons that contains all custom button Components. For whichever buttons we actually use in the app, we should move them to that folder.

[atharva-kashyap]

acknowledged! will keep this open so that we remember!

[amalnanavati]

Imo it's a bit strange that when one of these buttons are clicked, the border changes from rectangle to rounded rect. It is fine for this mock-up, but we'll want to fix that before actually implementing it.

[atharva-kashyap]

Yes, I recommend we keep it for this one but actually here are thoughts on that regard:

I completely agree with you! it is kind of weird that the button shape changes. But, originally i wanted the color to become light reddish to indicate it was selected. But, the problem is that since there are images inside this button, the red doesn't get shown. So, we need to play with the padding/borders to be able to show the red. Something to play around for when we actually implement!

[amalnanavati]

Tested it and functionality looks good! Good work :)

I am approving (all the changes I requested in the comments are small), but before merging you should do the following:

1. Merge main in, resolve any merge conflicts.
2. Address PR comments. For the comments that are not for this "mock-up" PR but should be addressed before implementing the final version, please leave them unresolved so we can quickly look back at those when we are implementing the final version.
3. Thoroughly test it (since the code may have changed slightly after merging main in).
4. Ensure the checks all pass. (Note that the checks have not run so far on your branch due to the merge conflict, they should run after you merge main in)
5. Squash and Merge this PR into main.

Good work :)

[atharva-kashyap]

thoroughly tested the functionality and it seems to be working according to expectations.

[amalnanavati]

@atharva-kashyap this line is where the bug was introduced. I'm not sure why the bug didn't appear on Chrome, but anyway I'll be removing this line in my next push to main