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 #122: Add FAN expansion mode #126

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

kiewietn
Copy link

@kiewietn kiewietn commented Aug 8, 2019

First time contributor checklist

Contributor checklist

  • Huawei Y3 CAG-L02, Android 8.1.0
  • Samsung Galaxy S5 SM-G900H, Android 6.0.1
  • Virtual device Nexus 5, Android 8.1.0
  • My contribution is fully baked and ready to be merged as is
  • I ensure that all the open issues my contribution fixes are mentioned in the commit message of my first commit
    using the Fixes #1234 syntax

Description

Add an expansion mode, where the items are arranged radially around the main FAB.
Items with label text set, will have the labels display below the mini fab item.
The radius length of the expansion mode can be changed from the xml layout file using the layout_constraintCircleRadius.

fab_expansion

Change FabWithLabelView to ConstraintLayout.
- This allows both horizontal and vertical expansion modes to show labels

Set layout params for each menu item to mimic expansion mode
for top/bottom/left and right

Expose hiding of label from FabWithLabelView
- For FAN expansion mode, the text is displayed below mini fab
- for right or left expansion modes, the text is always disabled. Same
as existing behaviour

Format code according to code style

Update sample app to show FAN expansion mode
@kiewietn kiewietn force-pushed the fan-expansion-mode branch from 1b956ce to dee3634 Compare August 8, 2019 08:39
@kiewietn
Copy link
Author

kiewietn commented Aug 8, 2019

Not really sure how to handle the overlapping text.
My thought was that clients will use shorter length labels, less action items or increase the expansion radius when text overlaps.

@kiewietn kiewietn marked this pull request as ready for review August 8, 2019 10:41
Copy link
Owner

@leinardi leinardi left a comment

Choose a reason for hiding this comment

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

Hi @kiewietn, thanks a lot for the Pull Request, but I'm afraid I will not be able to accept it until the issue with the overlapping/cut labels is solved: I'm absolutely positive that, if the current implementation will be released, I would get many bug reports for this problem.

@kiewietn
Copy link
Author

@leinardi, thanks for the comment.

To mitigate risk of overlapping labels, the maximum number of characters displayed on the label could be limited when using the FAN expansion mode.

The label length restrictions applied to the other expansion modes remain unchanged.

fan_expand_text_overlap

Please let me know if this is satisfactory.

@leinardi
Copy link
Owner

Hi @kiewietn, sorry for the late reply, busy week at work.

This is definitely an improvement but I'm still afraid that people will not understand this limitation and will keep opening issues complaining for the cut labels.

I would be fine to release this feature as experimental but I'm not sure how to do it with Java.
With Kotlin it would be possible to use the Experimental API marker.

I also notice a bug with the action items alignment:
image

It seems that, on your fork, they are not aligned to the main FAB anymore:
image

Upstream master doesn't have the issue:
image

@kiewietn
Copy link
Author

Thanks for coming back to me @leinardi .
I have fixed the alignment issue and added the limit to the label lengths.

Screenshot_1565944729

@stale
Copy link

stale bot commented Oct 15, 2019

This issue has been automatically marked as stale because it has not had activity in the last 60 days.

@stale stale bot added the Status: Stale label Oct 15, 2019
@stale
Copy link

stale bot commented Oct 15, 2019

This issue has been automatically marked as stale because it has not had activity in the last 60 days.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants