Create CarouselView widget - Part 1 #148094
Conversation
github-actions
bot
added
framework
There was a problem hiding this comment.
Do we have a ScrollConfiguration.scrollPhysicsOf(context) method? It feels a bit weird to have to use the context twice here. Do I file an issue?
There was a problem hiding this comment.
Filing an issue to add it SGTM!
There was a problem hiding this comment.
Do we follow-up with the other layouts (perhaps with factory constructors) in the future?
There was a problem hiding this comment.
Yes, another PR is created here: QuncCccccc#2
QuncCccccc
force-pushed
the
carousel_part1
branch
from
b344639 to
78c96cd
Compare
Piinks
left a comment
Piinks
left a comment
There was a problem hiding this comment.
Still need to review a bit more, will finish in the morning. Looking really good so far. 💯
| void dispose() { | ||
| _controller._detach(this); | ||
| if (widget.controller == null) { | ||
| _controller.dispose(); |
There was a problem hiding this comment.
Should this be _internalController? If _controller resolves as widget.controller, then I think the user is responsible for calling dispose, since they created it.
There was a problem hiding this comment.
Yes, for here _controller is _internalController because it happens when widget.controller is null:)
|
|
||
| double? getItemExtent() { | ||
| if (widget.itemExtent != null) { | ||
| final double screenExtent = switch (widget.scrollDirection) { |
There was a problem hiding this comment.
We should check this with a test, using MediaQuery means it is using the full screen, but that may not be what is available. If the Carousel is wrapped in Padding, or is in a row or column with other widgets, is this accurate?
There was a problem hiding this comment.
I have been wondering this as well. We'd have to resort to either a LayoutBuilder or RenderObject and cache the BoxConstraints. As an optimisation, we could skip that code path / LayoutBuilder|RenderBox if we have the itemExtent.
There was a problem hiding this comment.
That makes sense! I removed this method and checked the constraints in LayoutBuilder to make sure the ItemExtent is in the correct range.
There was a problem hiding this comment.
Filing an issue to add it SGTM!
QuncCccccc
left a comment
QuncCccccc
left a comment
There was a problem hiding this comment.
Thanks so much for all the comments! Just updated:)
| /// Called when one of the [children] is tapped. | ||
| final ValueChanged<int>? onTap; | ||
|
|
||
| /// The extent the children are forced to have in the main axis. |
There was a problem hiding this comment.
Oh this is set to nullable because the second part introduces a layoutWeights for Carousel.weighted. When layoutWeights is used, this is null. But for this PR, I should change it to be non-null property. Updated!
| } | ||
|
|
||
| class _CarouselState extends State<Carousel> { | ||
| late double? itemExtent; |
There was a problem hiding this comment.
Updated it to be a private field. I kept late because I changed itemExtent to be a non-nullable property. Once this PR is merged, the next PR(part 2) which adds layoutWeights will make itemExtent nullable and will remove late accordingly.
| } | ||
| } | ||
|
|
||
| double? getItemExtent() { |
There was a problem hiding this comment.
Removed this method based on the comments below. This value is constrained by using LayoutBuilder.
|
|
||
| double? getItemExtent() { | ||
| if (widget.itemExtent != null) { | ||
| final double screenExtent = switch (widget.scrollDirection) { |
There was a problem hiding this comment.
That makes sense! I removed this method and checked the constraints in LayoutBuilder to make sure the ItemExtent is in the correct range.
| Axis.vertical => MediaQuery.sizeOf(context).height, | ||
| }; | ||
|
|
||
| return clampDouble(widget.itemExtent!, 0, screenExtent); |
There was a problem hiding this comment.
Ah I constrained it because I think Carousel should at least be able to show one item completely, so the max extent should be no larger than its available space. Currently I didn't see any use case where each item is bigger than the constraints from the material specs. Just updated doc, but please let me know if this doesn't make sense:)
| /// Using a carousel controller helps to show which item is fully expanded on | ||
| /// the carousel list. |
There was a problem hiding this comment.
Oh this looks a little misleading. Updated. In the second part of Carousel, this controller should be able to show the first fully expanded item.
QuncCccccc
force-pushed
the
carousel_part1
branch
from
8ea7f73 to
3ea67ef
Compare
QuncCccccc
changed the title
There was a problem hiding this comment.
Following up on our chat about ScrollView as a base class. Since this does not use a scroll view, the concept of primary (PrimaryScrollController) and keyboard dismiss behavior is omitted. Since Carousel defaults to a horizontal scroll direction, this may be less of an issue, but just one to consider for the vertical edge cases in the spec. Maybe we can just file and issue to follow up on if folks want these features? This combination of Scrollable+Viewport could probably be switched out for a CustomScrollView.
I think the reason why PageView, which this current implementation is modeled after, does not utilize ScrollView as a base class is because PageView fills the entire viewport with its children. Or, one child of a PageView is displayed at a time. That and PageView primarily scrolls horizontally.
There was a problem hiding this comment.
Created a feature-request issue: #149765!
QuncCccccc
force-pushed
the
carousel_part1
branch
from
7b98c57 to
0fd9510
Compare
QuncCccccc
added
the
autosubmit
flutter/flutter@27e0656...4608a89 2024-06-06 [email protected] Fix InputDecorator suffixIcon color when in error and hovered (flutter/flutter#149643) 2024-06-06 [email protected] Roll Flutter Engine from 32c3b9b7cbe1 to 92d0cd9370f7 (1 revision) (flutter/flutter#149789) 2024-06-06 [email protected] Roll Flutter Engine from 0edca2e9d3d2 to 32c3b9b7cbe1 (2 revisions) (flutter/flutter#149786) 2024-06-06 [email protected] Roll Flutter Engine from f51e0ad3abbe to 0edca2e9d3d2 (8 revisions) (flutter/flutter#149785) 2024-06-06 [email protected] Roll Flutter Engine from f37733035060 to f51e0ad3abbe (3 revisions) (flutter/flutter#149778) 2024-06-05 [email protected] Remove abi key from local golden file testing (flutter/flutter#149696) 2024-06-05 [email protected] Fixes Router transaction to respect operation order (flutter/flutter#149763) 2024-06-05 [email protected] Send q once (flutter/flutter#149767) 2024-06-05 [email protected] Marks Mac_ios rrect_blur_perf_ios__timeline_summary to be unflaky (flutter/flutter#149729) 2024-06-05 [email protected] Add `contrastLevel` parameter to `ColorScheme.fromSeed` (flutter/flutter#149705) 2024-06-05 [email protected] Roll Flutter Engine from 11a32d43e3f6 to f37733035060 (11 revisions) (flutter/flutter#149770) 2024-06-05 [email protected] Remove unused code from an older test artifact. (flutter/flutter#149746) 2024-06-05 [email protected] Create CarouselView widget - Part 1 (flutter/flutter#148094) 2024-06-05 [email protected] [flutter_tools] Remove additional listener on VM service that simply logged incoming messages (flutter/flutter#149756) 2024-06-05 [email protected] Fix signature for TokenTemplate.updateFile (flutter/flutter#149673) 2024-06-05 [email protected] Remove temporary LayoutBuilder migration flag, defer `markNeedsLayout` (flutter/flutter#149637) 2024-06-05 [email protected] Revert "make output of flutter run web tests verbose (#149694)" (flutter/flutter#149766) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
|
I didn't find this |
|
Hi there,Thank you for the CarouselView widget—it's incredibly useful! However, I’ve encountered an issue that might need attention. It seems that gestures within the child widgets are disabled, possibly due to the use of the onTap(int) function. |
|
@houssemeddinefadhli1996 if you are experiencing an issue, then open an issue describing your problem with a minimum reproducible example. Here is not the correct place. |
|
Hi everyone. The team (usually) does not follow up on comments in closed PRs. Please open a new issue for bugs, see where's my commit for what build tag is on a given PR, or see the help channel on Discord for questions. |
5 participants
This PR is to create an "uncontained" Carousel.
scrolling_without_snapping.mov
The rest of the layouts can be achieved by using
Carousel.weighted: QuncCccccc#2. The branch ofCaorusel.weightedPR is based on this PR for relatively easer review. Will request reviews for the part 2 PR once this one is successfully merged in master.Pre-launch Checklist
///).