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.

Sign up for GitHub

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

Jump to bottom

feat: Product page redesign #6262

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from
Open

feat: Product page redesign #6262

wants to merge 6 commits into from

Conversation

PrimaelQuemerais
Copy link
Member

@PrimaelQuemerais PrimaelQuemerais commented Jan 21, 2025 •
edited
Loading

What

This PR introduces a new the layout for the product page (WIP)

Knowledge panels are now grouped in tabs which can be toggled and reordered.

Remaining tasks

  • Move reorder button to the end of the TabBar (instead of "For me" tab)
  • Save customizations locally (suggestions are welcome, how to conciliate hardcoded tabs and knowledge panel tabs)
  • Try to merge new generic reorder_bottom_sheet with ProductFooterSettings sheet
  • Implement UI elements from POC such as TabBar scroll hints, or border radius on selected tab indicator
  • Add ability to hide tabs

Screenshot

Simulator Screenshot - iPhone 16 Pro Max - 2025-01-21 at 15 40 42

Part of

@PrimaelQuemerais PrimaelQuemerais self-assigned this Jan 21, 2025
@teolemon teolemon requested a review from g123k January 21, 2025 15:02
@teolemon
Copy link
Member

tiny merge conflict @PrimaelQuemerais

g123k
g123k requested changes Jan 21, 2025
View reviewed changes
Copy link
Collaborator

@g123k g123k left a comment

Choose a reason for hiding this comment

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

I will do a second pass on your code, but here's a few suggestions

packages/smooth_app/lib/l10n/app_en.arb Outdated Show resolved Hide resolved
packages/smooth_app/lib/pages/product/product_page/header/product_tabs.dart Outdated Show resolved Hide resolved
packages/smooth_app/lib/pages/product/product_page/header/product_tabs.dart Outdated Show resolved Hide resolved
packages/smooth_app/lib/pages/product/product_page/header/product_tabs.dart Outdated Show resolved Hide resolved
packages/smooth_app/lib/pages/product/product_page/header/product_tabs.dart Outdated Show resolved Hide resolved
packages/smooth_app/lib/pages/product/product_page/header/reorder_bottom_sheet.dart Outdated Show resolved Hide resolved
packages/smooth_app/lib/pages/product/product_page/header/reorder_bottom_sheet.dart Outdated Show resolved Hide resolved
packages/smooth_app/lib/pages/product/product_page/header/reorder_bottom_sheet.dart Outdated Show resolved Hide resolved
packages/smooth_app/lib/pages/product/product_page/header/reorder_bottom_sheet.dart Outdated Show resolved Hide resolved
packages/smooth_app/lib/pages/product/product_page/header/reorder_bottom_sheet.dart Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Jan 22, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 0% with 187 lines in your changes missing coverage. Please review.

Project coverage is 6.02%. Comparing base (4d9c7fc) to head (d39ef88).
Report is 694 commits behind head on develop.

Files with missing lines Patch % Lines
...product/product_page/header/product_page_tabs.dart 0.00% 82 Missing ⚠️
...duct/product_page/header/reorder_bottom_sheet.dart 0.00% 70 Missing ⚠️
...b/pages/product/product_page/new_product_page.dart 0.00% 31 Missing ⚠️
...ges/smooth_app/lib/pages/product/summary_card.dart 0.00% 4 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##           develop   #6262      +/-   ##
==========================================
- Coverage     9.54%   6.02%   -3.52%     
==========================================
  Files          325     468     +143     
  Lines        16411   27482   +11071     
==========================================
+ Hits          1567    1657      +90     
- Misses       14844   25825   +10981

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@teolemon
Copy link
Member

@PrimaelQuemerais @g123k good to merge ?

@g123k
Copy link
Collaborator

g123k commented Jan 22, 2025

@PrimaelQuemerais @g123k good to merge ?

Not yet

@teolemon teolemon requested a review from g123k January 24, 2025 11:55
g123k
g123k requested changes Jan 24, 2025
View reviewed changes
packages/smooth_app/lib/pages/product/product_page/header/product_page_tabs.dart
label: tab.labelBuilder(context),
value: tab,
);
}).toList(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

.toList(growable:false)

packages/smooth_app/lib/pages/product/product_page/header/product_page_tabs.dart
value: tab,
);
}).toList(),
onTabChanged: (ProductPageTab tab) {},
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nothing here?

packages/smooth_app/lib/pages/product/product_page/header/product_page_tabs.dart
tabs.insert(
0,
ProductPageTab(
labelBuilder: (BuildContext c) =>
Copy link
Collaborator

Choose a reason for hiding this comment

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

To be consistent with the codebase, please name your variable "context"

packages/smooth_app/lib/pages/product/product_page/header/product_page_tabs.dart

tabs.add(
ProductPageTab(
labelBuilder: (BuildContext c) => knowledgePanelTitle.title,
Copy link
Collaborator

Choose a reason for hiding this comment

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

If you don't use the content, you can hide it with _

packages/smooth_app/lib/pages/product/product_page/header/product_page_tabs.dart
context,
items: tabs.map((ProductPageTab tab) {
return tab;
}).toList(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

.toList(growable:false)

packages/smooth_app/lib/pages/product/product_page/header/reorder_bottom_sheet.dart
Icon(
Icons.drag_handle,
color: SmoothColorsThemeExtension
.defaultValues()
Copy link
Collaborator

Choose a reason for hiding this comment

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

no

packages/smooth_app/lib/pages/product/product_page/header/reorder_bottom_sheet.dart
provider.reorder(oldIndex, newIndex);
onReorder(provider.items
.map((_ReorderableItem<T> item) => item.data)
.toList());
Copy link
Collaborator

Choose a reason for hiding this comment

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

growable:false

packages/smooth_app/lib/pages/product/product_page/header/reorder_bottom_sheet.dart
_ReorderableItem({required this.data, this.visible = true});

final T data;
bool visible;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you've missed a final here

packages/smooth_app/lib/pages/product/product_page/new_product_page.dart
SliverAppBar(
floating: false,
pinned: true,
leading: const SizedBox(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

EMPTY_WIDGET

packages/smooth_app/lib/pages/product/product_page/new_product_page.dart
controller: _tabController,
children: _tabs
.map((ProductPageTab tab) => tab.builder(upToDateProduct))
.toList(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

growable:false

@g123k
Copy link
Collaborator

g123k commented Jan 24, 2025

@teolemon I would hide tabs behind a dev mode.
We need more tabs for a general release

@teolemon
Copy link
Member

I count at least 3 to 4 tabs, health, environnement, report, possibly for me. Is it not enough ?

@g123k
Copy link
Collaborator

g123k commented Jan 24, 2025

I count at least 3 to 4 tabs, health, environnement, report, possibly for me. Is it not enough ?

If we want to publish a new release before the upcoming French TV show, the current page is OK (+ new icons).
This product page with tabs (which is not new, as stated in the title) needs to have "real" tabs content and not something like "Ok, it's just like before, but now with tabs". It's a terrible approach.

My goal for the past few weeks has been to have a better product for end users, and please don't revert to that state.
Tabs are OK, but only when they're fully ready.

@teolemon
Copy link
Member

teolemon commented Jan 25, 2025 •
edited
Loading

We can put it in dev mode or open beta feature for a while.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@g123k g123k g123k requested changes

Requested changes must be addressed to merge this pull request.

Assignees

@PrimaelQuemerais PrimaelQuemerais

Labels
🍎 iOS iOS specific issues or PRs 🌐 l10n 🥫 Product page summary card
Projects
🤳🥫 The Open Food Facts mobile app (Android & iOS)
Status: 🏗️ In Progress
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@PrimaelQuemerais @teolemon @codecov-commenter @g123k