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: fix overflow in heading mobile toolbar menu #291

Merged
merged 2 commits into from
Jul 12, 2023

Conversation

hyj1204
Copy link
Contributor

@hyj1204 hyj1204 commented Jul 5, 2023

To fix #285
I refactored the UI to make sure it always has three buttons on the heading mobile toolbar menu.
If the width of the screen is too small, the button texts will show in 2 lines

Pasted Graphic

@hyj1204 hyj1204 requested a review from LucasXu0 July 5, 2023 21:18
@hyj1204 hyj1204 self-assigned this Jul 5, 2023
@alihassan143
Copy link
Contributor

To fix #285 I refactored the UI to make sure it always has three buttons on the heading mobile toolbar menu. If the width of the screen is too small, the button texts will show in 2 lines

Pasted Graphic

instead of showing in 2 lines use wrap widget if screen is too small third heading will be rendered at second line that is more better looking than rendering text in 2 lines

@hyj1204
Copy link
Contributor Author

hyj1204 commented Jul 6, 2023

@alihassan143

The current layout rules in toolbar menu on mobile are:

  • The buttons on the toolbar menu take up all the space of the width of the screen
  • For every row will lay 2 or 3 buttons based on which toolbar item it is
  • Every button has the same space between them, and the rest of the space will be used to render buttons.

The main purpose of it is to make all components(buttons) take all the width without leaving some undefined width of spacing. In this way, the width of the screen control how wide it is for a button, it will look like this on most screen sizes.
image
If the width of screen is not wide enough to let the text inside button to show within one row, it will look like this
image

The way you suggest:

If we change to wrap, we have to rely on the text inside button to decide how wide the button it is, we will have no control over how much space will be left in a row.
On a wide screen:
Screenshot 2023-07-06 at 10 05 33 AM
On a narrow screen:
image

The way I implemented will fit most sizes of screens without unknown width spacing, which follows the same layout rules in other toolbar items menus on mobile.

@Xazin
Copy link
Collaborator

Xazin commented Jul 7, 2023

I am wondering if it would be fine to hide text where the icons themselves are so common that the user wouldn't have a problem interpreting it, on small screens.

@LucasXu0
Copy link
Collaborator

@hyj1204 Please check the comments.

@hyj1204
Copy link
Contributor Author

hyj1204 commented Jul 10, 2023

I am wondering if it would be fine to hide text where the icons themselves are so common that the user wouldn't have a problem interpreting it, on small screens.

Yes, for the heading part, the icons are easy to understand, I just feel we should try to keep the same UI style rules in all the mobile toolbar item menus. If we change all the buttons in the toolbar item menu to show icons only, I am not sure if all icons are easy to understand, also the icons will be centered in that case I think.

@codecov
Copy link

codecov bot commented Jul 10, 2023

Codecov Report

Merging #291 (e9d3fe2) into main (c5b5e64) will increase coverage by 2.12%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #291      +/-   ##
==========================================
+ Coverage   68.28%   70.40%   +2.12%     
==========================================
  Files         240      239       -1     
  Lines       10172     9948     -224     
==========================================
+ Hits         6946     7004      +58     
+ Misses       3226     2944     -282     
Impacted Files Coverage Δ
...ile/toolbar_items/heading_mobile_toolbar_item.dart 100.00% <100.00%> (ø)

... and 32 files with indirect coverage changes

@hyj1204 hyj1204 force-pushed the fix/overflow_in_heading branch from f346b3d to e9d3fe2 Compare July 10, 2023 21:09
@LucasXu0 LucasXu0 merged commit 719a2bf into AppFlowy-IO:main Jul 12, 2023
@hyj1204 hyj1204 deleted the fix/overflow_in_heading branch July 17, 2023 15:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

A RenderFlex overflowed by 39 pixels on the right in Heading Toolbar Item.
4 participants