-
-
Notifications
You must be signed in to change notification settings - Fork 279
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
[WIP] Making history page look more like the designs #690
Conversation
I have spent a lot of time trying to understand how the product lists currently work. It seems the The This means that the places where the app shows product lists aren't widgets composed of product-list widgets using surrounding containers and constructor parameters to control the rendering and behavior, but everything is opaquely controlled by the data container class Adding a new kind of product list into this structure, with a new look and behavior, would make both WDYT about this analys? Have I misunderstood something? To avoid this I have created a new widget called I intend it's detailed look and behavior to be controlled by constructor parameters as is the custom in Flutter, and provide it a It can then be used anywhere you want to display a page containing product lists in the more common Flutter way of composing widgets with constructor parameters instead of baking the display logic into the data container. Currently it only displays WDYT about this architectural idea. Does it make sense? |
@zond I did write
Don't hesitate to ask more questions. Btw I don't mind my code being thrown away (but for good reasons only), so don't be shy ;) |
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.
Oh I though this was an issue, not a PR.
The code looks clean. I did not run it.
What I can say is that in #349 I specifically added the timestamps. I don't know if they are still supposed to be there.
Excellent, thanks!
Still, the
Check.
Sounds good, I'll look at that!
That makes sense - but wouldn't that collide with how the
Check!
That's a sound approach - but I also find it a very sound approach to not throw code away but instead try to gently add new features or improvements into the current architecture :D I had no idea the response to my questions would be this quick, and your answers will surely help me add stuff in as gentle a way as possible 👍 |
@zond Your first step may be to extract the |
Alright, I refactored it a bit as per your recommendation. Do you think it's OK to redesign the |
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.
Hi @zond!
Not 100% convinced: please have a look at my comments.
I don't know what the product list looks like in Figma.
I don't know either how the concept of product list is supposed to evolve.
If the concepts of user defined lists, shopping lists and pantries are discarded, and if we only have history lists, I guess you can get rid of some code because you'll always have the following parameters: reorderable = false, timestamps = true, dismissible = true
.
My main question is: what do you really need if you want to "make history page look more like the designs", as the title of this PR suggests?
@@ -4,7 +4,7 @@ import 'package:provider/provider.dart'; | |||
import 'package:qr_code_scanner/qr_code_scanner.dart'; | |||
import 'package:smooth_app/data_models/continuous_scan_model.dart'; | |||
import 'package:smooth_app/pages/personalized_ranking_page.dart'; | |||
import 'package:smooth_app/pages/smooth_bottom_navigation_bar.dart'; | |||
import 'package:smooth_app/pages/smooth_bottom_navigation_bar.dart' as navbar; |
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.
What's the point? I assume that SmoothBottomNavigationBar
has an explicit name, that the name is long enough, and that it's used in a context where there's no ambiguity.
I won't reject the PR just for that, though. I guess it's about personal coding preferences.
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.
It was because for some reason the compiler complained that SmoothBottomNavigationBar
was defined in two files - I had no clue what caused it by this fixed it.
Now I can't repro that problem, so I removed the fix :)
); | ||
} | ||
|
||
static String _getDaysAgoLabel(final int daysAgo) { |
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.
While you're refactoring, you'd be better off creating a new file dedicated to "some time ago" labels. Maybe some_time_ago_helper.dart
, with String getDaysAgoLabel(final int daysAgo, final AppLocalizations appLocalizations)
. With actual translations, at least in TODO
s.
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.
Done.
final AppLocalizations appLocalizations = AppLocalizations.of(context)!; | ||
return Column( | ||
children: <Widget>[ | ||
AppBar( | ||
backgroundColor: Colors.white, | ||
foregroundColor: Colors.black, | ||
title: Text(appLocalizations.history), | ||
), | ||
Expanded(child: ProductListWidget(productList)), | ||
], | ||
); |
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.
The idea was still to use ProductListPage
, that would mainly use the new class ProductListWidget
where most of the page's code would have been moved.
The way the project evolved, ProductListPage
is now only used by the history page. If you don't use it in the history page anymore, everything you do in ProductListPage
is basically maintaining dead code.
And I'm not convinced that using a Column
with an AppBar
then a body (and without a navigation bar) is such a good idea compared to a standard Scaffold
.
My suggestion: let history page use ProductListPage
, that uses the new class ProductListWidget
.
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.
I'd be happy to remove ProductListPage
if it's unused, but see
I don't remember why I did it using Column
containing AppBar
... I changed it to a Scaffold
now.
Nesting 'pages' seems strange - if we want to use ProductListPage
only or the history page, then we could just rename it to HistoryPage
.
Also, see the image above - ProductList
is still used for the custom product lists (which I don't know if they are going away or not).
I don't know if they are going away or not (I'm just trying to pick up some tasks from @jasmeet0817), but even if they are not, I assume we will want the same styling and look on all lists of products. Mainly I'm talking about visual differences, how the different icons are laid out etc
|
63da93d
to
74a4ff9
Compare
…aking the current ProductListPage even more complex.
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.
Hi @zond!
Assuming that the Figma screenshots are correct - e.g. no timestamp displayed - and as you consider in HistoryPage
that you use the default version of ProductListWidget
(no reorderable; no dismissible, no timestamps), I would highly simplify the code and remove everything about reordeable, dismissible and timestamps (e.g. _Meta
).
Basically that would mean a simple ListView
. Much easier to read. Keeping "dismissible" would make sense, though (and setting it to true
for history), and is not in contradiction with the screenshots.
That being said:
- I recently implemented Add a "Clear all" button in the Product history page. #682 - please add this to your PR
- I implemented Revamp of the recently seen products screen #349 6 months ago - that was probably before the UX/UI refresh but @teolemon can you confirm that we should not display the history timestamps anymore?
Hi, sorry for late reply. They are going away indeed, we only need the product list in two places:
|
After discussion with @jasmeet0817 we decided that the simplest thing to do is to scrap this PR and make a new one that only changes the look of the row in the product list, since that is the most important difference between the UX designs and what's here currently. |
@zond @jasmeet0817 That makes sense, especially now that I've started to refactor the database AND trim all the pantry stuff en passant. The code looks much clearer. |
No description provided.