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

RefreshIndicator should appear from the top of the dragged widget where the [GlowingOverscrollIndicator] appears position. #71821

Closed
wants to merge 5 commits into from

Conversation

xu-baolin
Copy link
Member

Description

This change fixes that RefreshIndicator does not appear from the top of the dragged widget where the [GlowingOverscrollIndicator] appears position when custom the position of GlowingOverscrollIndicator.

import 'package:flutter/material.dart';

void main() {
  runApp(MaterialApp(home: BugReport(),));
}

class BugReport extends StatelessWidget {
  @override
  Widget build(BuildContext context) {
    double leadingPaintOffset = MediaQuery.of(context).padding.top + AppBar().preferredSize.height;
    return RefreshIndicator(
      onRefresh: () {
        return Future.delayed(const Duration(seconds: 3), () => print('onRefresh finished.'));
      },
      child: NotificationListener<OverscrollIndicatorNotification>(
        onNotification: (notification) {
          if (notification.leading) {
            notification.paintOffset = leadingPaintOffset;
          }
          return false;
        },
        child: CustomScrollView(
          slivers: [
            SliverAppBar(title: Text('Custom PaintOffset')),
            SliverToBoxAdapter(
              child: Container(
                color: Colors.amberAccent,
                height: 100,
                child: Center(child: Text('I love Flutter!', style: TextStyle(fontSize: 18.0),)),
              ),
            ),
            SliverFillRemaining(child: FlutterLogo()),
          ],
        ),
      ),
    );
  }
}

Related Issues

Fixes #71819

Tests

I added the following tests:

See files.

@flutter-dashboard flutter-dashboard bot added f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. labels Dec 7, 2020
@google-cla google-cla bot added the cla: yes label Dec 7, 2020
@darrenaustin darrenaustin self-requested a review December 11, 2020 00:50
/// [OverscrollIndicatorNotification.paintOffset] to custom the position of
/// GlowingOverscrollIndicator, you should insert the [NotificationListener] add between
/// the [RefreshIndicator] and scrollable widgets, otherwise, the RefreshIndicator
/// will cancel the notification bubbling:
Copy link
Member Author

Choose a reason for hiding this comment

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

should be otherwise, the RefreshIndicator can not receive the notification:

@xu-baolin
Copy link
Member Author

@darrenaustin Hi,
Welcome to review me :)

@darrenaustin
Copy link
Contributor

@xu-baolin, thanks for the contribution. Sorry it has taken me a while to look at this. I just looked at another related PR #72052 and it would appear that would solve this problem as well. Depending on how it lands, the app could just set the displacement (or edgeDistance if that is the decision for the PR) on the RefreshIndicator to match where the glow indicator is starting. Which I think would handle the same situation, but without the need for the RefreshIndicator to handle this case specifically.

Can you take a look at #72052 and see if it wouldn't solve the same issue by just updating your test program with:

    ...

    return RefreshIndicator(
      onRefresh: () {
        return Future.delayed(const Duration(seconds: 3), () => print('onRefresh finished.'));
      },
     displacement: leadingPaintOffset,

    ...

@xu-baolin
Copy link
Member Author

Thanks for reviewing.
I just take a look at that PR,
I think the situation is a bit different. My understanding is that if you change the position of the glow, the position of the indicator should automatically follow, instead of letting the user manually set it, and ensure that the values on both sides are the same to work properly. This is unfriendly.

@xu-baolin
Copy link
Member Author

The key is whether the indicator and glow should appear from the same position by default?
If the answer is yes, should we need also to change the position of the glow when changing the position of the indicator like #72052 ?

@Piinks
Copy link
Contributor

Piinks commented Oct 1, 2021

Hey @xu-baolin! I just came across this, would you still like to proceed with this change?

@xu-baolin
Copy link
Member Author

I have no idea, #72052 seems to help this,if you think so,just close this:)

@Piinks
Copy link
Contributor

Piinks commented Oct 1, 2021

Thank you! Much appreciated as always. :)

@Piinks Piinks closed this Oct 1, 2021
@darrenaustin
Copy link
Contributor

Indeed. Thank you both. I have been meaning to follow up on this for a while and it got buried in my task list.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels.
Projects
None yet
3 participants