Repository navigation
Expose padEnds on PageView widget - #78558
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat. If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
flutter-dashboard
Bot
added
the
framework
There was a problem hiding this comment.
The page snapping doesn't seem to work correctly
repro
/// Flutter code sample for PageView
// Here is an example of [PageView]. It creates a centered [Text] in each of the three pages
// which scroll horizontally.
import 'package:flutter/material.dart';
void main() => runApp(const MyApp());
/// This is the main application widget.
class MyApp extends StatelessWidget {
const MyApp({Key key}) : super(key: key);
static const String _title = 'Flutter Code Sample';
@override
Widget build(BuildContext context) {
return MaterialApp(
title: _title,
home: Scaffold(
appBar: AppBar(title: const Text(_title)),
body: const MyStatelessWidget(),
),
);
}
}
/// This is the stateless widget that the main application instantiates.
class MyStatelessWidget extends StatelessWidget {
const MyStatelessWidget({Key key}) : super(key: key);
@override
Widget build(BuildContext context) {
final PageController controller = PageController(initialPage: 0, viewportFraction: 0.5);
return PageView(
/// [PageView.scrollDirection] defaults to [Axis.horizontal].
/// Use [Axis.vertical] to scroll vertically.
scrollDirection: Axis.horizontal,
padEnds: false,
controller: controller,
children: const <Widget>[
Center(
child: Text('First Page'),
),
Center(
child: Text('Second Page'),
),
Center(
child: Text('Third Page'),
),
Center(
child: Text('Forth Page'),
),
Center(
child: Text('Fifth Page'),
)
],
);
}
}
|
I think is working, it is just that the paddings at start/end were removed and that is what I want to achieve: remove the padding at the start of the list.
|
I am not entirely sure what the correct behavior if snapping = true. Right now it seems like snapping is offset due to the none padding, and I don't think it can be useful. |
|
One of the use cases is in the docs itself:
I can share mine but I may take some time to share it here. |
My previous comment is confusing sorry. What I meant is I understand padEnds is useful, but the current snapping effect with padEnds = true looks wrong. We have to do something about it. |
This comment has been minimized.
This comment has been minimized.
|
[just-a-note] Also, it would be useful to customize the the paddings: being able to specify padding-right only or padding left and provide custom values to those paddings. |
|
@pedromassangocode I see, It looks like snapping will be snap the page to the beginning instead of to the center. I couldn't find a material design for the animation in this case, so I don't know what is the expected behavior, neither. We can either document this behavior in the PageView.pagesnapping, or default pagesnapping to false when padEnds is true and have an assert to make sure pagesnapping is false. Maybe @Piinks has other suggestions? |
Piinks
left a comment
Piinks
left a comment
There was a problem hiding this comment.
We can either document this behavior in the PageView.pagesnapping, or default pagesnapping to false when padEnds is true and have an assert to make sure pagesnapping is false.
I think either of these options sounds fine. I am not familiar with the expected behavior, but have not been able to find anything to indicate one way or the other.
I will go with the first option as the page snapping may be required as well (My current use-case actually needs the snapping feature) |
Piinks
left a comment
Piinks
left a comment
There was a problem hiding this comment.
It looks like this branch needs to be updated for merge conflicts before moving forward.
|
I will update this ASAP |
# Conflicts: # packages/flutter/lib/src/widgets/page_view.dart
chunhtai
left a comment
chunhtai
left a comment
There was a problem hiding this comment.
LGTM, except some comment suggestions
This comment has been minimized.
This comment has been minimized.
|
the test failure looks like the branch is not up to date |
|
Should be fine now |
|
This pull request is not suitable for automatic merging in its current state.
|
|
is it already release in the stable channel? I've update my flutter version but still no paddedEnd param in the PageView constructor |
Not yet. |
Fixes #78492
Fixes #52014
Pre-launch Checklist
///).