-
-
Notifications
You must be signed in to change notification settings - Fork 368
feat: Added price count badges #6493
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
base: develop
Are you sure you want to change the base?
feat: Added price count badges #6493
Conversation
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 @asim-sahoo!
Some things to change in your code, as you can read in my comments. And probably more, later.
Btw in your screenshots you don't show proofs but product images. I hope you didn't pollute the PROD server with irrelevant pictures - you tested prices on DEV/TEST, right?
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.
Please remove that file from the PR, it has nothing to do with your code.
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 changes were auto generated, I have reverted the changes but its still showing in the changed files.
same with the .gitignore file. I have no idea why its showing changed although there is no difference. can you please help me with this
@@ -26,6 +26,7 @@ class PriceProofPage extends StatefulWidget { | |||
|
|||
class _PriceProofPageState extends State<PriceProofPage> { | |||
List<Price>? _existingPrices; | |||
bool _isLoading = true; |
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.
_isLoading
has no purpose in your code, so please remove it.
), | ||
// Display price count badge on the detail screen | ||
if (!_isLoading && _existingPrices != null) | ||
Positioned( |
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.
Instead of a Stack
and a Positioned
, please just us a Badge
, cf. prices_card.dart
.
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.
@asim-sahoo ping
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.
yes
setState(() { | ||
_isLoading = false; | ||
}); |
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.
setState(() { | |
_isLoading = false; | |
}); |
setState(() { | ||
_isLoading = false; | ||
}); |
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.
setState(() { | |
_isLoading = false; | |
}); |
/// Created a simplified version of _PriceProofImage if needed for transition | ||
class PriceProofImage extends StatelessWidget { | ||
const PriceProofImage( |
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.
Just trying to understand: you're creating dead code, and you want it to be public instead of private, right?
} | ||
|
||
/// Widget to display a grid of proofs with price count badges | ||
class ProofGridItem extends StatefulWidget { |
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 may be old-fashioned but what is that code doing here in an unrelated file?
Assuming that this code is useful, it should be either in a specific file OR in a place where it makes sense, for instance where it's used, that is prices_proofs_page.dart
, where there is a grid.
|
||
Future<void> _loadPrices() async { | ||
if (PriceModel.isProofNotGoodEnough(widget.proof)) { | ||
setState(() { |
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.
For the record you're supposed to check if(mounted)
before setState
in an async code.
// Used our new ProofGridItem widget | ||
child: ProofGridItem( |
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.
Just put your change into the existing code, in that case in _PriceProofImage
.
setState(() { | ||
_existingPrices = prices.value.items ?? <Price>[]; | ||
_isLoading = false; | ||
}); |
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.
setState(() { | |
_existingPrices = prices.value.items ?? <Price>[]; | |
_isLoading = false; | |
}); | |
_existingPrices = prices.value.items ?? <Price>[]; | |
if (mounted) { | |
setState(() {}); | |
} |
…o/smooth-app into feature-price-proof-ui
…o/smooth-app into feature-price-proof-ui
This reverts commit 388b039.
@monsieurtanuki I’ve made all the requested changes. Since this is my first PR and I’m still learning, I might have made a few mistakes. I’d really appreciate your review. Thanks! |
Could you clarify this for me? I ran the app using Flutter and created an account to test the feature. Is there anything else I need to do to ensure the testing was done correctly? |
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.
@asim-sahoo Generally speaking, in this PR you're not asked to be inventive, you're asked to be lazy:
- just add a
Badge
on top of proof images in both proof grid and proof page cases - as this badge is being used in 2 places, it does make sense to make a specific class in a specific file for that
Oh, and please don't push again and again while someone's reviewing your code.
child: _PriceProofImage(proof, | ||
squareSize: squareSize), | ||
}, | ||
|
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.
Your code needs to be systematically formatted.
Have a look at Android Studio on how to make it automatic.
.gitignore
Outdated
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.
We don't want to see this file here.
Add some spaces here and here, make it as it used to be, whatever, this file doesn't belong here.
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've reverted all the changes to their original state, but they are still appearing in the changed section. Additionally, I don’t see any differences in the files so I dont know what to do. Could you please advise on how to resolve this?
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.
We don't want to see this file here.
Add some spaces here and here, make it as it used to be, whatever, this file doesn't belong here.
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've reverted all the changes to their original state, but they are still appearing in the changed section. Additionally, I don’t see any differences in the files so I dont know what to do. Could you please advise on how to resolve this?
/// Badge showing the number of prices with a payment icon | ||
class PriceBadge extends StatelessWidget { |
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.
Please keep it simple and just use a Badge
, as already mentioned. And forget about your additional icon.
If you really want to create your own widget - to be used in both proof grid and single proof pages:
- create a specific file, like
proof_badge.dart
- create a specific widget, like
ProofBadge
class _PriceProofImage extends StatefulWidget { | ||
const _PriceProofImage({ | ||
required this.proof, | ||
required this.onTap, |
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 of changing the parameters?
You did add crap data to the prices PROD server. Those are not proof images. Proofs are related to prices, they can be receipts or price tags. |
Apologies for that. I'll make sure to be more cautious in the future. I appreciate the clarification regarding proof images and the importance of distinguishing between PROD and TEST environments. Moving forward, I'll use the TEST server for development and testing to avoid adding unintended data. |
@monsieurtanuki I have made the changes. please review and tell me if further changes is required or not. |
Also the build is red because of: |
fixed |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #6493 +/- ##
==========================================
- Coverage 9.54% 5.88% -3.67%
==========================================
Files 325 496 +171
Lines 16411 29556 +13145
==========================================
+ Hits 1567 1739 +172
- Misses 14844 27817 +12973 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
What
ProofGridItem
for a cleaner layout and better readability.Screenshot
Fixes bug(s)