fix: RankingFloatingActionButton partially off-screen for some translations #5117

gmmoraes · 2024-03-13T03:17:29Z

What

Wrapped the RankingFloatingActionButton inside an Expanded widget and its child inside a FittedBox widget.

Screenshot

After:

Screen.Recording.2024-03-13.at.00.13.38.mov

Fixes bug(s)

5116

…d it's childs inside a FittedBox widget

codecov-commenter · 2024-03-13T03:21:25Z

Codecov Report

Attention: Patch coverage is 0% with 19 lines in your changes are missing coverage. Please review.

Project coverage is 8.62%. Comparing base (4d9c7fc) to head (72eef5d).
Report is 113 commits behind head on develop.

Files	Patch %	Lines
...p/lib/pages/product/common/product_query_page.dart	0.00%	11 Missing ⚠️
...pp/lib/widgets/ranking_floating_action_button.dart	0.00%	8 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           develop   #5117      +/-   ##
==========================================
- Coverage     9.54%   8.62%   -0.93%     
==========================================
  Files          325     329       +4     
  Lines        16411   16713     +302     
==========================================
- Hits          1567    1441     -126     
- Misses       14844   15272     +428

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

monsieurtanuki

Hi @gmmoraes!

To be honest I'm not 100% convinced by your fix :(

First: in the case of this specific issue/PR, screenshots would be a better option than video, if we want to check the alignments and the sizes of buttons.

Second: we already use AutoSizeText in other parts of the code, so that texts fit in a box. I would recommend using it, with maxLines: 2 for instance.

I would also summarize what we try to achieve here:

"top" behavior: a "floating action button like" button with an icon and a text that takes all the screen width (minus padding)
"bottom" behavior
- a row of the same height and width as in top scenario
- with an "icon+text" button the same height as before, with an AutoSizeText, that takes as much space as possible, width-wise
- then some padding
- then the up arrow button

gmmoraes · 2024-03-14T01:12:38Z

Hi @gmmoraes!

To be honest I'm not 100% convinced by your fix :(

First: in the case of this specific issue/PR, screenshots would be a better option than video, if we want to check the alignments and the sizes of buttons.

Second: we already use AutoSizeText in other parts of the code, so that texts fit in a box. I would recommend using it, with maxLines: 2 for instance.

I would also summarize what we try to achieve here:

"top" behavior: a "floating action button like" button with an icon and a text that takes all the screen width (minus padding)

"bottom" behavior

a row of the same height and width as in top scenario

with an "icon+text" button the same height as before, with an AutoSizeText, that takes as much space as possible, width-wise

then some padding

then the up arrow button

Hey @monsieurtanuki !

Thank you for the feedback!

I will take a look at the AutoSizeText package, and later this week, I will try to refactor the PR code following these instructions.

…ittedBox

gmmoraes · 2024-03-28T00:44:28Z

Hey @monsieurtanuki !

I've pushed a new commit. Please let me know if it's okay or if it requires any changes.

I've added AutoSizeText as the label property of RankingFloatingActionButton.
My main difficulty was managing a way for RankingFloatingActionButton's width constraints to affect its child widgets (more specifically AutoSizeText), because FloatingActionButton, and consequently its text, were not being resized despite RankingFloatingActionButton's width. To address this, I added RankingFloatingActionButton inside a Flexible widget so that Flexible can pass down its constraints. Additionally, I used a FittedBox widget to manage the resizing of child widgets.

Thanks in advance.

monsieurtanuki · 2024-03-28T07:13:40Z

Hi @gmmoraes!
Could you please attach 4 screenshots:

a full width FAB with a fake short text ("Hello")
a full width FAB with a fake 2-line text ("This is a test text that would fit in 2 lines bla bla bla")
a FAB on the left with a fake short text, and the icon button on the right
a full width FAB with a fake 2-line text, and the icon button on the right

gmmoraes · 2024-04-05T20:30:58Z

@monsieurtanuki , I am going send this weekend without fail!

gmmoraes · 2024-04-07T23:37:46Z

Hey @monsieurtanuki!

After running the tests, I am not sure if the current code solves this problem. Instead of breaking to a new line, it is reducing the font size.

Is it necessary to use FloatingActionButton in this widget? I am not sure why, but multiline is not working with it. However, by using ElevatedButton, I was able to achieve it.

With that being said, I have added below pictures of the current commit and pictures of using ElevatedButton instead of FloatingActionButton. Please let me know what you think.

Current commit Screen

a full width FAB with a fake short text ("Hello")

a full width FAB with a fake 2-line text ("This is a test text that would fit in 2 lines bla bla bla")

a FAB on the left with a fake short text, and the icon button on the right
a full width FAB with a fake 2-line text, and the icon button on the right

With ElevatedButton

a full width FAB with a fake short text ("Hello")

a full width FAB with a fake 2-line text ("This is a test text that would fit in 2 lines bla bla bla")

a FAB on the left with a fake short text, and the icon button on the right

a full width FAB with a fake 2-line text, and the icon button on the right

monsieurtanuki · 2024-04-09T11:11:04Z

Hi @gmmoraes! Thank you for your screenshots!

If I put the versions side by side, it's obvious that the ElevatedButton versions look better. And as we're not using the FloatingActionButton in a standard way anyway (because of the other "to the top" button), we may as well use ElevatedButton indeed. I'm not 100% satisfied with the height and the position, though.

current commit	ElevatedButton

Here are my suggestions to display relevant heights:

use ElevatedButton for RankingFloatingActionButton, with a fixed height of MINIMUM_TOUCH_SIZE
use ElevatedButton also for the "to the top" arrow button in product_query_page.dart, with a fixed height of MINIMUM_TOUCH_SIZE

Here are my suggestions to display consistent layouts:

align RankingFloatingActionButton to the right when alone, instead of centering it
align RankingFloatingActionButton then the arrow button to the right, in order to avoid the space between both buttons

@gmmoraes What do you think of that?

monsieurtanuki · 2024-05-06T08:25:07Z

@gmmoraes Still working on it?

gmmoraes · 2024-05-06T14:22:24Z

@monsieurtanuki , Apologies for the delayed response; I wasn't able to work on this task for the past month.

Regarding the suggestion, absolutely, it makes sense! Let's implement it as you've outlined. Thanks for the input!

I will try to implement the required changes this week. Next week, It will also begin my 10 days of holidays, so in a worst-case scenario, I will have a lot of time to work on this.

Does that sound okay?

monsieurtanuki · 2024-05-06T14:47:53Z

@gmmoraes No problem, take your time!

gmmoraes · 2024-05-07T15:11:11Z

Hi @gmmoraes! Thank you for your screenshots!

If I put the versions side by side, it's obvious that the ElevatedButton versions look better. And as we're not using the FloatingActionButton in a standard way anyway (because of the other "to the top" button), we may as well use ElevatedButton indeed. I'm not 100% satisfied with the height and the position, though.

current commit ElevatedButton

Here are my suggestions to display relevant heights:

use ElevatedButton for RankingFloatingActionButton, with a fixed height of MINIMUM_TOUCH_SIZE

use ElevatedButton also for the "to the top" arrow button in product_query_page.dart, with a fixed height of MINIMUM_TOUCH_SIZE

Here are my suggestions to display consistent layouts:

align RankingFloatingActionButton to the right when alone, instead of centering it

align RankingFloatingActionButton then the arrow button to the right, in order to avoid the space between both buttons

@gmmoraes What do you think of that?

@monsieurtanuki, regarding the previous suggestions, if you don't mind, I would like to confirm two pieces of information:

Concerning the RankingFloatingActionButton and BackToTopButton height issue from my previous images, was the problem that the BackToTopButton had a larger height?

Regarding the alignment requirement, should there be no horizontal space between the RankingFloatingActionButton and the BackToTopButton?

monsieurtanuki · 2024-05-07T16:10:17Z

@gmmoraes Your latest screenshot looks ok to me:

same height for both buttons, which brings uniformity to the UI
a space between both buttons
a padding to the left of the left and the right of the right
a text that is automatically resized as it is too big

…nkingFloatingActionButton

…ing alignment logic

…tton_overflow

gmmoraes · 2024-05-08T00:00:56Z

@monsieurtanuki , I've pushed a new commit. Please let me know if it's okay or if it requires any changes.

Implemented ElevatedButton with a fixed height of MINIMUM_TOUCH_SIZE inside RankingFloatingActionButton.
Changed BackToTopButton to have a fixed height of MINIMUM_TOUCH_SIZE.
Modified alignment logic for RankingFloatingActionButton inside product_query_page.dart, ensuring it aligns to the right when the BackToTopButton is not visible.

Cheers

monsieurtanuki

Thank you @gmmoraes for your latest version! I guess we're very close to the solution.
Please have a look at my comments, that are suggestions about simplifying the code.
Please also attach screenshots in the 4 interesting cases:

big label, no arrow button
big label, arrow button
small label, no arrow button
small label, arrow button

monsieurtanuki · 2024-05-08T05:50:21Z

packages/smooth_app/lib/pages/product/common/product_query_page.dart

+                ? MainAxisAlignment.end
+                : MainAxisAlignment.end,


In that case the test doesn't make sense anymore, does it?
It's always .end, right?

monsieurtanuki · 2024-05-08T05:53:14Z

packages/smooth_app/lib/pages/product/common/product_query_page.dart

-                          color: themeData.colorScheme.onSecondary,
+                      child: SizedBox(
+                        height: MINIMUM_TOUCH_SIZE,
+                        child: ElevatedButton(


I guess IconButton would make more sense and the code would be less verbose.
Would you please try it?

I guess IconButton would make more sense and the code would be less verbose. Would you please try it?

If we do use the IconButton, I believe it will need to be embedded inside an InkResponse widget for a circular ripple effect and inside a Material widget for background and elevation.

Perhaps something like this:

ClipOval( child: Material( elevation: 2.0, color: themeData.colorScheme.secondary, child: InkResponse( containedInkWell: true, child: SizedBox( height: MINIMUM_TOUCH_SIZE, child: IconButton( icon: const Icon( Icons.arrow_upward, ), color: themeData.colorScheme.onSecondary, onPressed: onPressed, ), ), ), ), );

Perhaps it could be added inside a private method that returns a widget:

Widget _getBackToTopButton(ThemeData themeData, void Function()? onPressed) => ClipOval( child: Material( elevation: 2.0, color: themeData.colorScheme.secondary, child: InkResponse( containedInkWell: true, child: SizedBox( height: MINIMUM_TOUCH_SIZE, child: IconButton( icon: const Icon( Icons.arrow_upward, ), color: themeData.colorScheme.onSecondary, onPressed: onPressed, ), ), ), ), );

What do you think?

@gmmoraes My assumption was that it was easy to use (and to maintain) an IconButton in order to display a clickable disk with an icon inside. Easier than an ElevatedButton.
If it's not the case, don't waste time with the hard to read/maintain solution you've just suggested, and ignore my suggestion about using an IconButton.

monsieurtanuki · 2024-05-08T05:55:44Z

packages/smooth_app/lib/pages/product/common/product_query_page.dart

-                        barcodes: _model.displayBarcodes,
-                        title: widget.name,
+              Expanded(
+                child: Padding(


The padding is pointless here and probably causes confusion. Please try to remove it.

monsieurtanuki · 2024-05-08T05:58:01Z

packages/smooth_app/lib/widgets/ranking_floating_action_button.dart

+                  style: ButtonStyle(
+                    shape: MaterialStateProperty.all<RoundedRectangleBorder>(
+                      const RoundedRectangleBorder(
+                          borderRadius: BorderRadius.all(Radius.circular(40))),


Suggested change

borderRadius: BorderRadius.all(Radius.circular(40))),

borderRadius: CIRCULAR_RADIUS,

monsieurtanuki · 2024-05-08T06:00:00Z

packages/smooth_app/lib/widgets/ranking_floating_action_button.dart

-              label: Text(AppLocalizations.of(context).myPersonalizedRanking),
-              onPressed: onPressed,
-            ),
+            Flexible(


Generally speaking I'm not sure we need that Row and especially that SizedBox.

gmmoraes · 2024-05-10T02:39:33Z

@monsieurtanuki ,
Thank you for your feedback! I'll review the code and work on the suggestions.
As for the screenshots, agreed, I will attach them in my next comment for the following version.

Cheers,

… and removed redundant Padding

…tton_overflow

gmmoraes · 2024-05-13T18:51:55Z

@monsieurtanuki , I've pushed a new commit. Please let me know if it's okay or if it requires any changes.
Below, I have added the requested images.

Edit: Each text below is a toggle button, to show the image just need to click on it.

big label, no arrow button

big label, arrow button

medium label, no arrow button

medium label, arrow button

small label, no arrow button

small label, arrow button

Cheers!

monsieurtanuki

Thank you @gmmoraes for the good work!
Perhaps there will be minor cosmetic changes later, but now we have a stable code that manages typical cases with different text lengths.

Gabriel Moraes added 2 commits March 12, 2024 23:11

Bug: wrapping RankingFloatingActionButton inside a Expanded widget an…

efeef68

…d it's childs inside a FittedBox widget

Merge branch 'develop' into bug/ranking_float_action_button_overflow

53f2d7b

gmmoraes requested a review from a team as a code owner March 13, 2024 03:17

github-actions bot assigned gmmoraes Mar 13, 2024

github-actions bot added the 🥫 Product page label Mar 13, 2024

gmmoraes changed the title ~~Fix/RankingFloatingActionButton partially off-screen for some translations~~ fix: RankingFloatingActionButton partially off-screen for some translations Mar 13, 2024

monsieurtanuki reviewed Mar 13, 2024

View reviewed changes

Gabriel Moraes added 2 commits March 27, 2024 21:23

fix: Adding AutoSizeText to FloatingActionButton and adding it to a F…

d854a60

…ittedBox

fix: returning RankingFloatingActionButton class comments

fca916e

teolemon added the layout label Apr 8, 2024

Gabriel Moraes and others added 3 commits May 7, 2024 20:22

refactor: Replacing FloatingActionButton for ElevatedButton inside Ra…

7e97946

…nkingFloatingActionButton

fix: Limiting BackToTopButton height to MINIMUM_TOUCH_SIZE and adjust…

f775dbd

…ing alignment logic

Merge branch 'openfoodfacts:develop' into bug/ranking_float_action_bu…

78a5eed

…tton_overflow

monsieurtanuki reviewed May 8, 2024

View reviewed changes

Gabriel Moraes added 2 commits May 13, 2024 15:19

fix: Changed the property of a widget from conditional to fixed value…

263a015

… and removed redundant Padding

fix: Refactor widget structure from Row to Container

7a44eb8

Merge branch 'openfoodfacts:develop' into bug/ranking_float_action_bu…

72eef5d

…tton_overflow

monsieurtanuki approved these changes May 14, 2024

View reviewed changes

monsieurtanuki merged commit 9075fdc into openfoodfacts:develop May 14, 2024
6 checks passed

github-actions bot mentioned this pull request May 14, 2024

chore(develop): release 4.15.0 #5106

Open

monsieurtanuki mentioned this pull request May 25, 2024

RankingFloatingActionButton partially off-screen for some translations #5116

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: RankingFloatingActionButton partially off-screen for some translations #5117

fix: RankingFloatingActionButton partially off-screen for some translations #5117

gmmoraes commented Mar 13, 2024

codecov-commenter commented Mar 13, 2024 •

edited

monsieurtanuki left a comment

gmmoraes commented Mar 14, 2024

gmmoraes commented Mar 28, 2024 •

edited

monsieurtanuki commented Mar 28, 2024

gmmoraes commented Apr 5, 2024 •

edited

gmmoraes commented Apr 7, 2024

monsieurtanuki commented Apr 9, 2024

monsieurtanuki commented May 6, 2024

gmmoraes commented May 6, 2024 •

edited

monsieurtanuki commented May 6, 2024

gmmoraes commented May 7, 2024

monsieurtanuki commented May 7, 2024

gmmoraes commented May 8, 2024

monsieurtanuki left a comment

monsieurtanuki May 8, 2024

monsieurtanuki May 8, 2024

gmmoraes May 11, 2024

monsieurtanuki May 11, 2024

monsieurtanuki May 8, 2024

monsieurtanuki May 8, 2024

monsieurtanuki May 8, 2024

gmmoraes commented May 10, 2024

gmmoraes commented May 13, 2024 •

edited

monsieurtanuki left a comment

	borderRadius: BorderRadius.all(Radius.circular(40))),
	borderRadius: CIRCULAR_RADIUS,

fix: RankingFloatingActionButton partially off-screen for some translations #5117

fix: RankingFloatingActionButton partially off-screen for some translations #5117

Conversation

gmmoraes commented Mar 13, 2024

What

Screenshot

Fixes bug(s)

codecov-commenter commented Mar 13, 2024 • edited

Codecov Report

monsieurtanuki left a comment

Choose a reason for hiding this comment

gmmoraes commented Mar 14, 2024

gmmoraes commented Mar 28, 2024 • edited

monsieurtanuki commented Mar 28, 2024

gmmoraes commented Apr 5, 2024 • edited

gmmoraes commented Apr 7, 2024

Current commit Screen

With ElevatedButton

monsieurtanuki commented Apr 9, 2024

monsieurtanuki commented May 6, 2024

gmmoraes commented May 6, 2024 • edited

monsieurtanuki commented May 6, 2024

gmmoraes commented May 7, 2024

monsieurtanuki commented May 7, 2024

gmmoraes commented May 8, 2024

monsieurtanuki left a comment

Choose a reason for hiding this comment

monsieurtanuki May 8, 2024

Choose a reason for hiding this comment

monsieurtanuki May 8, 2024

Choose a reason for hiding this comment

gmmoraes May 11, 2024

Choose a reason for hiding this comment

monsieurtanuki May 11, 2024

Choose a reason for hiding this comment

monsieurtanuki May 8, 2024

Choose a reason for hiding this comment

monsieurtanuki May 8, 2024

Choose a reason for hiding this comment

monsieurtanuki May 8, 2024

Choose a reason for hiding this comment

gmmoraes commented May 10, 2024

gmmoraes commented May 13, 2024 • edited

monsieurtanuki left a comment

Choose a reason for hiding this comment

codecov-commenter commented Mar 13, 2024 •

edited

gmmoraes commented Mar 28, 2024 •

edited

gmmoraes commented Apr 5, 2024 •

edited

gmmoraes commented May 6, 2024 •

edited

gmmoraes commented May 13, 2024 •

edited