-
Notifications
You must be signed in to change notification settings - Fork 0
Controls to Navigate Between Meal States #30
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…b_interface into raidak/meal_state_controls
…nalrobotics/feeding_web_interface into raidak/meal_state_controls
…nalrobotics/feeding_web_interface into raidak/meal_state_controls
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
feedingwebapp/public/robot_state_imgs/move_to_mouth_position.svg
Outdated
Show resolved
Hide resolved
feedingwebapp/public/robot_state_imgs/move_above_plate_position.svg
Outdated
Show resolved
Hide resolved
feedingwebapp/public/robot_state_imgs/move_to_staging_position.svg
Outdated
Show resolved
Hide resolved
feedingwebapp/public/robot_state_imgs/move_to_staging_position.svg
Outdated
Show resolved
Hide resolved
Here are some additional details regarding this PR's deliverables: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work! It is quite close to being done :) I left a few small comments in-line, and have a few more here:
- I realized that we should never have the robot move back to BiteAcquisition without first going to BiteSelection. Because say the robot attempted a bite acquisition and failed. In the process, it may have moved the food around on the plate. Therefore, the old mask the user specified for the food item is no longer valid, and the user has to re-select a bite to get a new mask given the new locations of the foods. See Issue Should we add a "Reselect Bite" option to BiteAcquisitionCheck? #39 for additional discussion about this.
- As a result of this, the button in
BiteAcquisitionCheck
to try again should instead go to "MovingAbovePlate" -- could you change that state transition and icon? - Once you do this, you'll notice that
move_to_bite_acquisition.svg
is no longer used. So you can delete if you want. If you decide not to delete it (just in case we decide to use it in the future), you should make the changes to it I mentioned in the in-line comments.
- As a result of this, the button in
- You'll notice that the branch is out-of-date with
main
(see the bottom of this PR). Before you next mark it as ready for re-review, could you "Update Branch"? After you do that on the web, you'll have togit pull
locally to get that change. - I created Issue Implement "back" button functionality after the user has clicked pause #40 for the "Back" button functionality. That can be focused on after this PR is merged in.
feedingwebapp/public/robot_state_imgs/move_to_staging_position_footer.svg
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good work, this all looks good to me.
There is one thing I forgot in my last review: adding an Acknowledgements.md
file with the icons we used. As I had mentioned when we were first creating the icons, it is important to keep track of the icons we use in order to acknowledge them. Can you add that file, and add follow the instructions here or here to acknowledge all the icons we used?
[Describe this pull request. Link to relevant GitHub issues, if any.]
This PR includes issue #10 deliverables as detailed in the following:
[Explain how this pull request was tested, including but not limited to the below checkmarks.]
I tested the implementations by running the app screens in my local Safari browser and ensuring:
Before creating a pull request
npm run format
Before merging a pull request
Squash and Merge
)