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

Added restaurant card implementation #1398

Open
wants to merge 5 commits into
base: ui/redesign
Choose a base branch
from

Conversation

abutuc
Copy link

@abutuc abutuc commented Nov 26, 2024

Closes #1274

Added the redesign implementation of the restaurant card. The card has as input parameters "restaurantName", "restaurantType", "menuItems", "isFavorite" and "onFavoriteToggle" callback function, which allows us to abstract the logic from the component. Additionally, this card returns a Generic Card whose child is the actual restaurant card content.

Restaurant Card

Note: the background color and the elevation property are being inherited from the Generic Card implementation, so when the Generic Card implementation has these issues fixed, the Restaurant Card should look like the mock version.

Review checklist

  • Terms and conditions reflect the current change
  • Contains enough appropriate tests
  • If aimed at production, writes a new summary in whatsnew/whatsnew-pt-PT
  • Properly adds an entry in changelog.md with the change
  • If PR includes UI updates/additions, its description has screenshots
  • Behavior is as expected
  • Clean, well-structured code

@abutuc abutuc requested a review from DGoiana November 26, 2024 12:47
Copy link

codecov bot commented Nov 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 12%. Comparing base (dfcb039) to head (57c4fdd).
Report is 1 commits behind head on ui/redesign.

Additional details and impacted files
@@             Coverage Diff             @@
##           ui/redesign   #1398   +/-   ##
===========================================
  Coverage           12%     12%           
===========================================
  Files              271     271           
  Lines             7341    7341           
===========================================
  Hits               812     812           
  Misses            6529    6529           

@DGoiana
Copy link
Collaborator

DGoiana commented Nov 26, 2024

@abutuc can you add screenshots of the result?

@DGoiana
Copy link
Collaborator

DGoiana commented Nov 26, 2024

Use TabBar in order to build and implement the top bar

@thePeras thePeras requested a review from a team November 30, 2024 20:09
@DGoiana DGoiana mentioned this pull request Dec 9, 2024
7 tasks
@abutuc abutuc force-pushed the redesign/restaurant-card branch from 0cdab5d to 455a59f Compare January 2, 2025 00:32
@abutuc abutuc requested review from vitormpp and thePeras January 2, 2025 16:14
@abutuc
Copy link
Author

abutuc commented Jan 2, 2025

I added the requested changes. After performing the rebase the final look of the card component is:

restaurant_card

I left the UniIcon implementation on the restaurant_card.dart file, but only for the favorite button icons (the others are being passed as arguments). Maybe in the future it would be nice to have a uni-ui global UniIcon widget so that we can use Duotone Phosphor Icons without needing to save the svg file.

I hope I addressed each suggested change and sorry for the delay!
@DGoiana @thePeras @vitormpp

@DGoiana
Copy link
Collaborator

DGoiana commented Jan 2, 2025

@abutuc good work here, the card looks pretty neat! Overall, the code looks good, only 2 things are itching my brain. First, in RestaurantCardHeader try to make the distance between the title and the top equal to the distance between the title and the gray line. Second, I do not understand the need to have UniIcon, can you clarify it?

@abutuc
Copy link
Author

abutuc commented Jan 2, 2025

@abutuc good work here, the card looks pretty neat! Overall, the code looks good, only 2 things are itching my brain. First, in RestaurantCardHeader try to make the distance between the title and the top equal to the distance between the title and the gray line. Second, I do not understand the need to have UniIcon, can you clarify it?

changed

@DGoiana what do you think? This is the best I could do to even out, I can't seem to reduce the upper space between the text and the top border of the container. I am not sure if anyone could help me out in this detail.
To make it visually more vertically centered, I just added some bottom padding to the container widget. I attempted to vertically center the Row widget and each child, but nothing worked!

Regarding the UniIcon part, I realized I didn't actually need it in the RestaurantCardHeader widget, so I removed it. I also realized that there's no need for the UniIcon widget overall, since we can just use the PhosphorIcon and pass it a PhosphorDuotoneIconData.

@abutuc abutuc requested a review from DGoiana January 2, 2025 18:01
@DGoiana
Copy link
Collaborator

DGoiana commented Jan 2, 2025

@abutuc it looks great! Regarding the UniIcon, the class is being introduced in #1379. We can later add those heart icons into it.

Copy link
Member

@thePeras thePeras left a comment

Choose a reason for hiding this comment

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

Nice Work!! ✅ 🛫

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants