Skip to content

Support New and Custom FAB Locations #51465

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
Merged
fluttergithubbot merged 10 commits into from
Apr 25, 2020
Merged

Support New and Custom FAB Locations #51465

fluttergithubbot merged 10 commits into from
Apr 25, 2020

Conversation

@pennzht
Copy link
Member

@pennzht pennzht commented Feb 26, 2020 •
edited
Loading

Description

This PR also adds the following FAB locations, and provides a class StandardFABLocation and mixins to support user-defined FAB locations.

  • centerTop
  • miniCenterTop
  • miniEndTop
  • startFloat
  • miniStartFloat
  • miniCenterFloat
  • miniEndFloat
  • startDocked
  • miniStartDocked
  • miniCenterDocked
  • miniEndDocked

This PR does not implement multiple FAB locations.

There are now 18 predefined FAB locations. Each one is the combination of 3 properties:

  • Horizontal location — start, center, end;
  • Vertical location — top, float, docked;
  • Expected size — mini, not mini.

All 18 combinations are predefined.
The locations miniCenterTop and miniCenterDocked are equivalent in effect to centerTop and centerDocked, respectively, but they are defined separately.

Related Issues

Closes #15122.

Tests

I added the following tests:

  • Tests for new FAB locations
    • Tests if every FloatingActionButtonLocation is correctly displayed.
    • Includes 1 test for each location.
    • Includes 2 tests for right-to-left support.
  • Moves involving new locations
    • Tests if floating action buttons can animate between locations, both old and new.
    • Includes tests with custom animators.
  • Tests for custom FAB locations
    • Tests if user-defined FAB locations can correctly position the FAB, including in right-to-left environments.

Screenshots

FloatingActionButtonLocation.endTop
endTop

FloatingActionButtonLocation.startFloat
startFloat

FloatingActionButtonLocation.miniStartDocked
miniStartDocked

FloatingActionButtonLocation.miniEndFloat
miniEndFloat

FloatingActionButtonLocation.miniCenterFloat
miniCenterFloat

A user-defined FAB location, 50 pixels away from endFloat.
almostEndFloat

A user-defined FAB location, 1/4 the way between endFloat and startFloat.
quarterEndFloat

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I signed the CLA.
  • I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I updated/added relevant documentation (doc comments with ///).
  • All existing and new tests are passing.
  • The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Did any tests fail when you ran them? Please read Handling breaking changes.

  • No, no existing tests failed, so this is not a breaking change.

@pennzht pennzht self-assigned this Feb 26, 2020
@fluttergithubbot fluttergithubbot added f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. labels Feb 26, 2020
@pennzht pennzht changed the title Fab complete new Support New and Custom FAB Locations Feb 26, 2020
@pennzht pennzht mentioned this pull request Feb 26, 2020
Closed
9 tasks
rami-a
rami-a approved these changes Feb 27, 2020
View reviewed changes
Copy link
Contributor

@rami-a rami-a left a comment

Choose a reason for hiding this comment

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

LGTM with one possible suggestion

packages/flutter/lib/src/material/floating_action_button_location.dart Outdated
Copy link
Contributor

@rami-a rami-a Feb 27, 2020

Choose a reason for hiding this comment

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

You mentioned in the description that miniCenterTop and miniCenterDocked are not defined since they are just the same as their non-mini versions.

I think it might be more clear if they were defined and just pointed to the same private class, to make the API more consistent. Just a suggestion.

Copy link
Member Author

@pennzht pennzht Apr 3, 2020

Choose a reason for hiding this comment

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

Thank you for the suggestion! I think we can ask Hans about this when he reviews.

Copy link
Member Author

@pennzht pennzht Apr 21, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Done:
miniCenterTop
miniCenterDocked

phillipet added a commit to phillipet/ata that referenced this pull request Mar 17, 2020
@phillipet
fix new position FloatingActionButtonLocation
880c7f6 
 solved at flutter/flutter#51465 (20 days ago)
@phillipet phillipet mentioned this pull request Mar 17, 2020
Merged
HansMuller
HansMuller approved these changes Apr 8, 2020
View reviewed changes
Copy link
Contributor

@HansMuller HansMuller left a comment

Choose a reason for hiding this comment

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

Great change; really nice job of factoring. Because we're providing so many FAB location alternative, we're going to need to help developers out by providing more visual documentation than is usual. That extra documentation can be taken care of in a separate PR.

packages/flutter/lib/src/material/floating_action_button_location.dart Outdated
Copy link
Contributor

@HansMuller HansMuller Apr 8, 2020

Choose a reason for hiding this comment

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

This is a good summary of the class's structure, but it doesn't really explain what it's for. I'd suggest inserting a overview sentence (followed by a blank line of course) like: A base class that simplifies building [FloatingActionButtonLocation]s when used with mixins [FabTopOffsetY] ... just enumerate them all here.

Copy link
Member Author

@pennzht pennzht Apr 21, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Done.

packages/flutter/lib/src/material/floating_action_button_location.dart Outdated
Copy link
Contributor

@HansMuller HansMuller Apr 8, 2020

Choose a reason for hiding this comment

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

Please use a public FabLocation class for this example

Copy link
Member Author

@pennzht pennzht Apr 21, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Done.

packages/flutter/lib/src/material/floating_action_button_location.dart Outdated
Copy link
Contributor

@HansMuller HansMuller Apr 8, 2020

Choose a reason for hiding this comment

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

Just one sentence explaining what this custom location is doing that's special (adjusting the button's horizontal origin ...), would help.

Copy link
Member Author

@pennzht pennzht Apr 21, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Done.

packages/flutter/lib/src/material/floating_action_button_location.dart Outdated
Copy link
Contributor

@HansMuller HansMuller Apr 8, 2020

Choose a reason for hiding this comment

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

All of the mixins refer to an exported FloatingActionButtonLocation, just like this one refers to FloatingActionButtonLocation.startTop. That's good. However for a developer to quickly understand what we mean by "startTop", I think the API docs for the exported locations will need a small screenshot. Something like the ones included in this PR's description. That kind of doc can be added in a separate PR.

Copy link
Member Author

@pennzht pennzht Apr 21, 2020

Choose a reason for hiding this comment

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

I submitted an issue as a feature request:
#55321

packages/flutter/lib/src/material/floating_action_button_location.dart Outdated
Copy link
Contributor

@HansMuller HansMuller Apr 8, 2020

Choose a reason for hiding this comment

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

Move the doc above the method here and elsewhere (please review the entire file for this kind of typo/error).

Copy link
Member Author

@pennzht pennzht Apr 9, 2020

Choose a reason for hiding this comment

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

This method has an @override. Should the doc be above the @override or below it?

Copy link
Contributor

@HansMuller HansMuller Apr 9, 2020

Choose a reason for hiding this comment

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

above the override

Copy link
Member Author

@pennzht pennzht Apr 21, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Done.

packages/flutter/lib/src/material/floating_action_button_location.dart Outdated
Copy link
Contributor

@HansMuller HansMuller Apr 8, 2020

Choose a reason for hiding this comment

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

This method and the following one could be static

Copy link
Member Author

@pennzht pennzht Apr 21, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Done.

@pennzht pennzht mentioned this pull request Apr 21, 2020
Closed
@pennzht pennzht requested a review from HansMuller April 21, 2020 19:55
HansMuller
HansMuller approved these changes Apr 25, 2020
View reviewed changes
Copy link
Contributor

@HansMuller HansMuller left a comment

Choose a reason for hiding this comment

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

Nice job on this one! LGTM

@fluttergithubbot fluttergithubbot merged commit 0c4a659 into flutter:master Apr 25, 2020
pennzht added a commit that referenced this pull request Apr 25, 2020
@pennzht
Revert "Support New and Custom FAB Locations (#51465)"
64a9cb3 
This reverts commit 0c4a659.
@pennzht pennzht mentioned this pull request Apr 25, 2020
Closed
@cad0p
Copy link

cad0p commented May 16, 2020

Hi! There is an issue with bottom FAB locations in Flutter Web: on Android, the banner that prompts you to install the app hides the FAB button!

(I haven't tested the bottom nav bar but that could have an issue as well)

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 31, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

3 more reviewers

@clocksmith clocksmith clocksmith approved these changes

@HansMuller HansMuller HansMuller approved these changes

@rami-a rami-a rami-a approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@pennzht pennzht

Labels

f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Request for left-aligned floating Action Button

7 participants

@pennzht @cad0p @clocksmith @HansMuller @rami-a @googlebot @fluttergithubbot