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.

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: make card bottom prompt visible #1488

Merged
merged 5 commits into from
Apr 19, 2022

Conversation

AshAman999
Copy link
Member

What

  • made the bottom prompt visible in the card

Screenshot

image

Fixes bug(s)

@AshAman999 AshAman999 requested a review from a team as a code owner April 3, 2022 20:44
@codecov-commenter
Copy link

Codecov Report

Merging #1488 (9e2efe6) into develop (9c507b8) will not change coverage.
The diff coverage is 0.00%.

@@           Coverage Diff           @@
##           develop   #1488   +/-   ##
=======================================
  Coverage     9.04%   9.04%           
=======================================
  Files          158     158           
  Lines         6431    6431           
=======================================
  Hits           582     582           
  Misses        5849    5849           
Impacted Files Coverage Δ
...ges/smooth_app/lib/pages/product/summary_card.dart 0.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9c507b8...9e2efe6. Read the comment docs.

@@ -136,7 +136,9 @@ class _SummaryCardState extends State<SummaryCard> {
child: Center(
child: Text(
AppLocalizations.of(context)!.tab_for_more,
style: Theme.of(context).primaryTextTheme.bodyText1,
style: Theme.of(context).primaryTextTheme.bodyText1?.copyWith(
Copy link
Member

Choose a reason for hiding this comment

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

What about .textTheme.bodyText1 ?

Copy link
Member Author

Choose a reason for hiding this comment

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

@VaiTon thought of keeping the blue color so as to make that look like hyperlink to make it obvious that it can be clicked

@teolemon
Copy link
Member

teolemon commented Apr 4, 2022

The button cropped halfway thru is ugly (but the fact that the border is not aware of the content seems a tough problem)

@teolemon teolemon added this to the V1 milestone Apr 4, 2022
@teolemon teolemon linked an issue Apr 4, 2022 that may be closed by this pull request
@monsieurtanuki
Copy link
Contributor

The button cropped halfway thru is ugly (but the fact that the border is not aware of the content seems a tough problem)

@teolemon Perhaps adding some alpha would make the transition smoother.

@AshAman999
Copy link
Member Author

image

@teolemon and @VaiTon what about this ui change ?

@VaiTon
Copy link
Member

VaiTon commented Apr 4, 2022

What about the primary as the background, so that it could be seen as a button

@AshAman999
Copy link
Member Author

What about the primary as the background, so that it could be seen as a button

image
??

@M123-dev
Copy link
Member

M123-dev commented Apr 5, 2022

The button cropped halfway thru is ugly (but the fact that the border is not aware of the content seems a tough problem)

Actually no button is supposed to be on the card when it is in the scanner, but the same problem occurrences for texts like allergies or ingredients

What about the primary as the background, so that it could be seen as a button

The thing is, it isn't a button. It's more a hint that the whole card can be tapped or even swiped up

@VaiTon
Copy link
Member

VaiTon commented Apr 5, 2022

The thing is, it isn't a button. It's more a hint that the whole card can be tapped or even swiped up

You're right, but it emphatizes that it can be tapped. I'm for the primary background

@teolemon
Copy link
Member

Any progress on this one ?

@AshAman999
Copy link
Member Author

Any progress on this one ?

The color wasn't finalised, is blue okay for that field?

@AshAman999
Copy link
Member Author

@abhay1821 can you look into this issue I'm having troubles with my pc and have sent it for repair, might take 2 - 3 days,
Meanwhile can you look into this pr?

@M123-dev
Copy link
Member

Fixed some merge conflicts @AshAman999

Copy link
Member

@M123-dev M123-dev left a comment

Choose a reason for hiding this comment

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

Lets merge as is, looks good @AshAman999

@M123-dev M123-dev merged commit c94ff24 into openfoodfacts:develop Apr 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

Visual issues on the scan card
6 participants