Skip to content
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

Add Home Screen View #56

Merged
merged 31 commits into from Oct 4, 2021
Merged

Add Home Screen View #56

merged 31 commits into from Oct 4, 2021

Conversation

RafaelPiloto10
Copy link
Member

@RafaelPiloto10 RafaelPiloto10 commented Sep 30, 2021

Parent (merge these first): #55

Description:

Implements the main home page and adds some core functionality.

item.dart - Implements the data structure for each food item
menu_data.dart - Implements the core functionality that will interface with our DCT backend (mocked for now)

home_screen.dart - Implements the home screen page

Screenshots

Home View

Screen Shot 2021-10-03 at 10 50 50 PM

Selecting Grilled Chicken

Screen Shot 2021-10-04 at 2 28 32 PM

@RafaelPiloto10 RafaelPiloto10 added the review ready This issue is ready for review! label Sep 30, 2021
@RafaelPiloto10 RafaelPiloto10 added this to the Sprint 2 milestone Sep 30, 2021
@RafaelPiloto10 RafaelPiloto10 self-assigned this Sep 30, 2021
@RafaelPiloto10 RafaelPiloto10 added this to In progress in Cheese & Quackers Board via automation Sep 30, 2021
Copy link
Member

@DDVD233 DDVD233 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please merge the previous one first. The two have too much stuff in common...

Cheese & Quackers Board automation moved this from In progress to Review in progress Sep 30, 2021
@RafaelPiloto10
Copy link
Member Author

Yes, I should have made my description clearer - merge the parents first

@DDVD233
Copy link
Member

DDVD233 commented Sep 30, 2021

It looks like merging the previous PR does not affect this one. Interesting.

@DDVD233
Copy link
Member

DDVD233 commented Sep 30, 2021

I guess I will review it anyway.

@RafaelPiloto10
Copy link
Member Author

I need to merge the updates into my branch and then push again

Copy link
Member

@DDVD233 DDVD233 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On iPhone 13 it gives me a text-overflow warning. I think this is due to the name being too long, and there is no space (<0px) for the dot textbox.
This is not that important, as the app still works, but there are a few ways to get around that. One quick fix might be to make the text a bit smaller. You can also try setting the left textbox to a fixed maximum size, and wrap them in multiple lines if the name gets too long.
image

quack_app/lib/main.dart Outdated Show resolved Hide resolved
Copy link
Member

@DDVD233 DDVD233 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall you have done an awesome job. The code is nice and neat. The structure is clear. Tests are complete. Great.

@RafaelPiloto10
Copy link
Member Author

RafaelPiloto10 commented Sep 30, 2021

On iPhone 13 it gives me a text-overflow warning. I think this is due to the name being too long, and there is no space (<0px) for the dot textbox. This is not that important, as the app still works, but there are a few ways to get around that. One quick fix might be to make the text a bit smaller. You can also try setting the left textbox to a fixed maximum size, and wrap them in multiple lines if the name gets too long.

I fixed this by adding an AutoSizeText package. I figured this was the most responsive approach given screen sizes. We provide a max font size, and the AutoSizeText will find the font size that best fits the constraints as you suggested.

Copy link
Member

@DDVD233 DDVD233 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work!

quack_app/lib/screens/homepage/home_screen.dart Outdated Show resolved Hide resolved
Copy link
Contributor

@brendacano brendacano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't look through the code yet but from the images please change the star at the bottom to fit the hearts.

quack_app/lib/components/navbar.dart Outdated Show resolved Hide resolved
quack_app/lib/main.dart Show resolved Hide resolved
quack_app/test/components/navbar_test.dart Show resolved Hide resolved
@brendacano brendacano mentioned this pull request Oct 3, 2021
Cheese & Quackers Board automation moved this from Review in progress to Reviewer approved Oct 3, 2021
Copy link
Member

@DDVD233 DDVD233 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All good!

@DDVD233 DDVD233 mentioned this pull request Oct 4, 2021
@RafaelPiloto10
Copy link
Member Author

@DDavid233, what was the latest commit you made? Let's try to not inter-mix between pull requests. If you'd like something changed, request changes that I can add or merge. As the owner of this pull request, I am responsible for the code I merge. I cannot be responsible for that if we are intermixing code we add. I don't see tests for the latest commit which appears to be the home screen but on favorite_screen.dart?

@DDVD233
Copy link
Member

DDVD233 commented Oct 4, 2021

@RafaelPiloto10 That is my mistake. I was trying to revert it.

@DDVD233
Copy link
Member

DDVD233 commented Oct 4, 2021

I meant to start a new branch but ended up pushing to your branch instead. Sorry about that.

@DDVD233
Copy link
Member

DDVD233 commented Oct 4, 2021

I was trying to push a hard reset, but apparently, I don't have permission to do that. I pushed a revert commit instead. Everything should be back to where it was before.

@RafaelPiloto10
Copy link
Member Author

@DDavid233, no worries! Thank you for fixing it :)

@RafaelPiloto10 RafaelPiloto10 linked an issue Oct 4, 2021 that may be closed by this pull request
11 tasks
quack_app/lib/screens/homepage/home_screen.dart Outdated Show resolved Hide resolved
quack_app/lib/screens/homepage/item_screen.dart Outdated Show resolved Hide resolved
@RafaelPiloto10 RafaelPiloto10 merged commit acbb2b4 into dev Oct 4, 2021
Cheese & Quackers Board automation moved this from Reviewer approved to Ready for Demo Oct 4, 2021
@RafaelPiloto10 RafaelPiloto10 deleted the home-screen branch October 4, 2021 18:30
@Foxworth22 Foxworth22 moved this from Ready for Demo to Done in Cheese & Quackers Board Oct 13, 2021
@Foxworth22 Foxworth22 removed the review ready This issue is ready for review! label Nov 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

[2pts] Home Screen
4 participants