-
-
Notifications
You must be signed in to change notification settings - Fork 313
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
fix: moved robotoff questions on product page #3549
fix: moved robotoff questions on product page #3549
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.
Hey @prathamsoni11! I see that beside the product page file you also modified the "standalone" screen. Is that useful to this PR or we could split it into another PR? |
Yes, definitely. We should keep both for hunger games. Also, what happens when you are done with the questions. Does the card disappear or is the congrats page opened. |
@VaiTon @M123-dev This is a past screen recording ignore the arrangement and background colour of the buttons. In this, all the components are covered. Screenrecorder-2022-12-09-18-12-37-631.mp4 |
Looks great, there are some merge conflicts, and we should keep the inline separate from the full page, we'll need both |
I didn't get what exactly you are saying. Can you explain it briefly? |
Yes @prathamsoni11 , so if you go into the devmodes and activate hungergames then you can find it in Settings-> contribute-> Hungergames There we need the full Page variant of these questions, so we need two versions
Some parts of the logic can surely be shared between them, but I'd recommend to have different widgets per version |
Is there any design ready that needs to be implemented or should I have to use the old one? |
There are two ways to implement:-
|
I would suggest to leave the old implementation untouched and create a new widgets for the inline card on the product page @prathamsoni11 |
@M123-dev This is the result after creating a new Widget. Is this good? |
Ohh sorry if I was unclear in my request, I didn't mean the full page to be new created. We need two pages:
We have to find a way so that both of them work, as expected. That is why I recommended to leave the full page hungergames unchanged and just create a new widget for the product page I hope my explanation can be understood |
ok, I will restore hungergames page to default and implement a new widget for the product page. |
Perfect, that is exactly what I had in mind |
Done👍🏻 |
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.
Heyy @prathamsoni11 looks amazing, I'm sure this will lead to way more usage.
Added a few comments all of them very small ones to be fixed in a few seconds.
But there are some merge conflicts which have to be resolved before we can merge
style: Theme.of(context).textTheme.bodyText1!.apply( | ||
color: isDarkMode ? Colors.white : Colors.black, | ||
), |
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.
Are those changes necessary, this will likely look out of place in the coming AMOLED mode
style: TextStyle( | ||
color: isDarkMode ? Colors.white : Colors.black, | ||
), |
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.
Same here, but if these colors are indeed needed, please use Theme.of(context).textTheme.bodyText1!.apply(
as above
Color _yesBackground = Colors.green; | ||
Color _noBackground = Colors.red; | ||
const Color _maybeBackground = Colors.white; | ||
const Color _yesNoTextColor = Colors.white; | ||
Color _maybeTextColor = Colors.grey.shade700; |
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.
Can you move them inside the ProductQuestionAnswersOptions
also all of them could be const, right?
); | ||
} | ||
|
||
Widget _buildQuestionShimmer() => Shimmer.fromColors( |
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.
Still love this shimmer effect ;D
import 'package:smooth_app/query/product_questions_query.dart'; | ||
import 'package:smooth_app/query/questions_query.dart'; | ||
|
||
class ProductQuestionPage 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 wouldn't call it a page, maybe ProductQuestionWidget or something like that (also the filename)
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.
good 👍
@prathamsoni11 we need a rebase on develop |
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'm going to merge this now. Thanks for your work @prathamsoni11
@M123-dev @prathamsoni11 this has been merged, but I can't see the new behaviour in the app. Am I missing something ? |
What
Screenshot
Part of