Skip to content

Conversation

@shihaohong
Copy link
Contributor

Unfortunately, it seems like #47616 broke some internal tests. We will have to revert until we can figure out what caused the change.

@fluttergithubbot fluttergithubbot added f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. labels Feb 10, 2020
@shihaohong
Copy link
Contributor Author

cc/ @filaps I'll try to figure out what's broken since you cannot view the internal tests and share my findings here when I can

Copy link
Contributor

@dnfield dnfield left a comment

Choose a reason for hiding this comment

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

LGTM

@shihaohong shihaohong merged commit 67826bd into master Feb 10, 2020
@dnfield dnfield deleted the revert-47616-bugfix/snackbar branch February 10, 2020 22:59
@dnfield
Copy link
Contributor

dnfield commented Feb 10, 2020

For a little more details: on an internal test we found that the snackbar was coming in a bit lower than expected after this change.

Hopefully we can add a similar test to the Flutter repo covering the same case.

@shihaohong
Copy link
Contributor Author

shihaohong commented Feb 10, 2020

In an example where SnackBarBehavior.floating is used, the SnackBar shows up lower than expected with the new change. With the old change, it appears a little higher.

Test case:

import 'package:flutter/material.dart';

void main() {
  runApp(MyApp());
}

class CustomSnackBar {
  void show(BuildContext context) {
    final snackBar = message(context);
    Scaffold.of(context).hideCurrentSnackBar();
    Scaffold.of(context).showSnackBar(snackBar);
  }

  SnackBar message(BuildContext context) {
    return SnackBar(
      behavior: SnackBarBehavior.floating,
      content: FlutterLogo(),
    );
  }
}

class MyApp extends StatelessWidget {
  @override
  Widget build(BuildContext context) {
    return MaterialApp(
      home: Scaffold(
        body: Builder(
          builder: (BuildContext context) => Center(
            child: OutlineButton(
              onPressed: () {
                final accountSnackBar = CustomSnackBar();
                accountSnackBar.show(context);
              },
              child: Text('Tap me'),
            ),
          ),
        ),
      ),
    );
  }
}

@shihaohong
Copy link
Contributor Author

shihaohong commented Feb 10, 2020

I just realized that the change is intentional. @filaps, how do you reproduce the case in your second screenshot where the SnackBar seems clipped by the bottom nav bar / space saved for the FAB rect?

Also, it may seem like we might have to make a breaking change announcement of some sort because while it is a fix, other developers might rely on the implementation or work around it. Still trying to figure out what to do in that case.

cc/ @Hixie

@Zazo032
Copy link
Contributor

Zazo032 commented Feb 11, 2020

I just realized that the change is intentional. @filaps, how do you reproduce the case in your second screenshot where the SnackBar seems clipped by the bottom nav bar / space saved for the FAB rect?

Also, it may seem like we might have to make a breaking change announcement of some sort because while it is a fix, other developers might rely on the implementation or work around it. Still trying to figure out what to do in that case.

cc/ @Hixie

To reproduce the clipped SnackBar, you have to drag it a bit to the bottom (like if you were going to dismiss it). #43716

@shihaohong
Copy link
Contributor Author

shihaohong commented Feb 11, 2020

Right now, I think we should initiate the breaking change process. @filaps, I could write the design doc and help land the changes in the steps required. If you'd prefer to take ownership of some parts of the process, please let me know!

@dnfield
Copy link
Contributor

dnfield commented Feb 11, 2020

IMO this does not require a design doc - just a migration guide and a flag so that we can soft migrate downstream users.

@dnfield
Copy link
Contributor

dnfield commented Feb 11, 2020

I say this because I suspect this is a change most users want without knowing they want it, and the migration guide is to update your golden image testing :)

shihaohong pushed a commit that referenced this pull request Feb 11, 2020
…tingActionButton positioning (#47616)" (#50516)"

This reverts commit 67826bd.
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 1, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants