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.

Sign up for GitHub

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

Jump to bottom

fix: secondary button now differentiable issue #2988 #3171

Merged
merged 6 commits into from
Nov 2, 2022
Merged

fix: secondary button now differentiable issue #2988 #3171

merged 6 commits into from
Nov 2, 2022

Conversation

Akashsri3bi
Copy link
Contributor

What

Fixed as requirements

Screenshot_2022-10-15-01-52-21-68 1

@Akashsri3bi Akashsri3bi requested a review from a team as a code owner October 17, 2022 18:30
M123-dev
M123-dev reviewed Oct 18, 2022
View reviewed changes
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.

Heyy, @Akashsri3bi this change looks good but there is one problem, on some devices this text is too long to fit in a row. We already fixed this by allowing to pass a positiveAction and negativeAction like here:

smooth-app/packages/smooth_app/lib/pages/product/common/product_dialog_helper.dart

Line 57 in 77569bf

positiveAction: SmoothActionButton(

It would be good to use this instead of the row

@Akashsri3bi
Copy link
Contributor Author

Actually in the issue one of the project maintainers advised that in row it will look better, no worries I will fix it as required..

@M123-dev M123-dev linked an issue Oct 18, 2022 that may be closed by this pull request
Discussion: The secondary button is hard to differentiate from text #2988
Closed
@M123-dev
Copy link
Member

I know @Akashsri3bi it was actually me 😆

#2988 (comment)

Didn't knew that we have these as parameters for our SmoothAlertDialog I thought we used a own widget for it instead 😅

@Akashsri3bi
Copy link
Contributor Author

Okay , No worries😁 I will fix it

@Akashsri3bi
Copy link
Contributor Author

@M123-dev I think now it's fine with recent changes !

AshAman999
AshAman999 requested changes Oct 20, 2022
View reviewed changes
packages/smooth_app/lib/pages/preferences/user_preferences_contribute.dart Outdated
appLocalizations.contribute_improve_ProductsToBeCompleted,
child: Container(
padding:
const EdgeInsets.symmetric(horizontal: 8, vertical: 5),
Copy link
Member

Choose a reason for hiding this comment

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

I believe there are constants already declared, if I remember MEDIUM_SPACE, like these, instead of hardcoding 8, 5, and 23 it would make sense to use a constant or maybe in case you need a new one declare that as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So Should I replace constant values ? and use MEDIUM_SPACE instead or something else

Copy link
Member

Choose a reason for hiding this comment

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

Yes @Akashsri3bi

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay great !

Copy link
Member

Choose a reason for hiding this comment

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

While thinking about it again, using a SmoothActionButton would simplify this even more @Akashsri3bi, the thinking is to just have one place to change values in one place if we decide to change something on the theme

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made it to negativeactionbutton , what is going wrong in tests not able to understand ?

Copy link
Member

Choose a reason for hiding this comment

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

That's the golden tests in action for now, can you send the screenshot page in light and dark theme

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Screenshot_2022-10-26-23-07-53-90 1

Screenshot_2022-10-26-23-07-03-66 1

Sure @AshAman999

Copy link
Member

Choose a reason for hiding this comment

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

Now the contents aren't readable,
Can we tweak the negative action button's properties to make it readable,

Looking after this, we were much better with the early version you did,
Any suggestions here @M123-dev >?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried to do some changes in class property but It does not works , What would you recommend ?

@codecov-commenter
Copy link

Codecov Report

Merging #3171 (b8e0619) into develop (adb7716) will decrease coverage by 0.01%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           develop   #3171      +/-   ##
==========================================
- Coverage     6.19%   6.17%   -0.02%     
==========================================
  Files          248     252       +4     
  Lines        12439   12495      +56     
==========================================
+ Hits           771     772       +1     
- Misses       11668   11723      +55
Impacted Files Coverage Δ
...pages/preferences/user_preferences_contribute.dart 4.60% <0.00%> (-0.26%) ⬇️
...ges/smooth_app/lib/pages/product/summary_card.dart 0.00% <0.00%> (ø)
...mooth_app/lib/helpers/robotoff_insight_helper.dart 0.00% <0.00%> (ø)
...th_app/lib/cards/data_cards/image_upload_card.dart 0.00% <0.00%> (ø)
...ib/cards/product_cards/product_image_carousel.dart 0.00% <0.00%> (ø)
...b/pages/preferences/user_preferences_dev_mode.dart 0.00% <0.00%> (ø)
packages/smooth_app/lib/pages/question_page.dart
...smooth_app/lib/query/robotoff_questions_query.dart
...es/smooth_app/lib/pages/hunger_games/congrats.dart 0.00% <0.00%> (ø)
... and 5 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@Akashsri3bi
Copy link
Contributor Author

@AshAman999 @M123-dev secondary action button now looks appropriate

M123-dev
M123-dev approved these changes Nov 2, 2022
View reviewed changes
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.

Sorry for this late response, but looks great, thanks @Akashsri3bi

@Akashsri3bi
Copy link
Contributor Author

Sorry for this late response, but looks great, thanks @Akashsri3bi

Thanks very much ! 🙂

@M123-dev M123-dev merged commit cfb5137 into openfoodfacts:develop Nov 2, 2022
@github-actions github-actions bot mentioned this pull request Nov 2, 2022
Merged
@Akashsri3bi Akashsri3bi deleted the ak2 branch November 2, 2022 17:12
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AshAman999 AshAman999 AshAman999 requested changes

@M123-dev M123-dev M123-dev approved these changes

Assignees

@Akashsri3bi Akashsri3bi

Labels
🎨 Design
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Discussion: The secondary button is hard to differentiate from text
5 participants
@Akashsri3bi @M123-dev @codecov-commenter @AshAman999 @teolemon