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
Update scan page as per the UX spec (well, roughly) #659
Conversation
This reverts commit d50b416.
final EdgeInsets qrScannerPadding = EdgeInsets.only( | ||
top: | ||
(availableScanHeight - scannerSize.height) / 2 + buttonRowHeight); | ||
final double viewFinderBottomOffset = carouselHeight / 2.0; |
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.
not entirely sure if this is needed.... (?)
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 added by @slava-sh a while ago, but I don't know what it's exact purpose is
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 will try and test on an iOS device, (device_preview renders fine without this offset but maybe there's some issue on iOS)
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 code looks good. Do you know what happend to pub_upgrade.sh
it looks like its size changed
final Size scannerSize = Size( | ||
screenSize.width * 0.6, | ||
screenSize.width * 0.33, | ||
); |
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.
Is it right that both use the width
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.
yeah, they are supposed to be proportional to one another
final EdgeInsets qrScannerPadding = EdgeInsets.only( | ||
top: | ||
(availableScanHeight - scannerSize.height) / 2 + buttonRowHeight); | ||
final double viewFinderBottomOffset = carouselHeight / 2.0; |
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 added by @slava-sh a while ago, but I don't know what it's exact purpose is
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 @jasmeet0817!
I don't know if it was in the UX spec, but not being able to clear the list when you have 1 product sounds strange.
Beyond that, nothing that shocks me in your code.
), | ||
); | ||
return AnimatedOpacity( | ||
opacity: areButtonsRendered(model) ? 0.8 : 0.0, |
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 guess it would make sense to display the "clear" button when we only have one product.
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.
hmm good point, I'm still not sold on this app behavior to be honest, because both buttons are very interdependent in functionality and the functionality is super unclear to the user unless they experiment with the buttons.
- The Compare button compares all products in scan history
- Clear button clears scan history
So I personally think it makes sense to pair and show them together, but I will bring it up with Pierre, Stephane and others this week
Small phone:
Large phone:
No buttons:
Note: The text is intentionally not localized yet, I'm waiting for these button names to become final from product's side.