Expose controller for PaginatedDataTable by amanv8060 · Pull Request #100005 · flutter/flutter

amanv8060 · 2022-03-12T10:04:59Z

Exposes controller for PaginatedDataTable

If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I signed the CLA.
I listed at least one issue that this PR fixes in the description above.
I updated/added relevant documentation (doc comments with ///).
I added new tests to check the change I am making, or this PR is test-exempt.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

flutter-dashboard · 2022-03-12T10:05:03Z

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 (don't just cc him here, he won't see it! He's on Discord!).

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.

amanv8060 · 2022-03-13T22:49:17Z

I don't think that primary is required, or should I add it as well or write a manual comment instead of using the template?
Also will add tests for scroll_controller scrolling.
Thanks.

Piinks

Hey @amanv8060! Thanks for the contribution.

I think adding primary as well here will be beneficial to the goal the issue is describing. It is the PrimaryScrollController (PSC) that the default keyboard shortcuts check to see if they can activate any scrolling actions, so both controller and primary sound like reasonable additions here.

Fun fact, in #99547, the PSC does not kick in automatically because the PSC only automatically attaches to vertical scroll views. 😉

Let me know if you need guidance for adding tests. Thanks!

amanv8060 · 2022-03-22T22:23:56Z

Sure, will try to do it this weekend.

amanv8060 · 2022-04-02T11:14:11Z

@Piinks I have added the tests for

if psc gets attached
if we can scroll the data table through controller

packages/flutter/test/material/paginated_data_table_test.dart

packages/flutter/lib/src/material/paginated_data_table.dart

packages/flutter/test/material/paginated_data_table_test.dart

packages/flutter/lib/src/material/paginated_data_table.dart

Piinks · 2022-04-05T21:00:51Z

packages/flutter/test/material/paginated_data_table_test.dart

This does not actually verify which scrollable used the scroll controller, like the other test below.

Since we never pass the controller to the second scrollable(footer), can we safely assume that it is always DataTable that uses the provided ScrollController?

That is a great question! I imagine it is possible the user may want access to one controller or the other at some point. Does the issue specify which scrollable is needed for access? If so, it may require we have clearer parameters here that indicate which controller this is.

Issue mentions that it needs controller for contents of PaginatedDataTable. I assume that it is for main content.

Hi @Piinks, Should I expose both the controllers?

I am not sure, if the issue is not clear we should ask the person that filed it for clarity. We shouldn't assume t know what they mean. Can you ask in the issue you are trying to resolve?

Sure, I did ask and the author only needs the controller for DataTable.

packages/flutter/test/material/paginated_data_table_test.dart

Piinks

LGTM! Thank you!

Piinks · 2022-04-27T23:28:55Z

This will need a second review, I have sought one out.

)

flutter-dashboard bot added f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. labels Mar 12, 2022

HansMuller changed the title ~~Exposed controller~~ Exposed controller for PaginatedDataTable Mar 18, 2022

HansMuller changed the title ~~Exposed controller for PaginatedDataTable~~ Expose controller for PaginatedDataTable Mar 18, 2022

HansMuller requested a review from Piinks March 18, 2022 22:01

Piinks reviewed Mar 22, 2022

View reviewed changes

amanv8060 force-pushed the 99547 branch from 2f12536 to 0eb37cc Compare April 2, 2022 09:53

amanv8060 force-pushed the 99547 branch from d698177 to b8ec3ed Compare April 2, 2022 11:52

amanv8060 commented Apr 2, 2022

View reviewed changes

packages/flutter/test/material/paginated_data_table_test.dart Outdated Show resolved Hide resolved

Piinks reviewed Apr 5, 2022

View reviewed changes

packages/flutter/lib/src/material/paginated_data_table.dart Outdated Show resolved Hide resolved

packages/flutter/test/material/paginated_data_table_test.dart Outdated Show resolved Hide resolved

amanv8060 force-pushed the 99547 branch from 978f4be to c39fcfb Compare April 5, 2022 20:34

Piinks reviewed Apr 5, 2022

View reviewed changes

packages/flutter/lib/src/material/paginated_data_table.dart Outdated Show resolved Hide resolved

Piinks reviewed Apr 5, 2022

View reviewed changes

Piinks added the f: scrolling Viewports, list views, slivers, etc. label Apr 7, 2022

amanv8060 added 7 commits April 20, 2022 01:52

Exposed controller

d0baf21

add test for psc and controller

11ca07c

fix typo

893d71e

fix:typo

f951523

fix: primary should be false by default

7e526bb

make primary nullable

8d90b05

fix:test

4cef9e0

amanv8060 force-pushed the 99547 branch from bed1c43 to 4cef9e0 Compare April 19, 2022 20:49

amanv8060 requested a review from Piinks April 26, 2022 14:26

Piinks approved these changes Apr 27, 2022

View reviewed changes

cbracken approved these changes May 9, 2022

View reviewed changes

Piinks added the waiting for tree to go green label May 9, 2022

fluttergithubbot merged commit 0ff0aff into flutter:master May 9, 2022

engine-flutter-autoroll mentioned this pull request May 9, 2022

Roll Flutter from b305eb633780 to 0ff0affb3a9d (36 revisions) flutter/packages#1858

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 9, 2022

0ff0aff Expose controller for PaginatedDataTable (flutter/flutter#100005

a993d2b

)

engine-flutter-autoroll mentioned this pull request May 9, 2022

Roll Flutter from b305eb633780 to ff136cba7fe0 (37 revisions) flutter/packages#1859

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 9, 2022

0ff0aff Expose controller for PaginatedDataTable (flutter/flutter#100005

6ed07d6

)

amanv8060 deleted the 99547 branch May 9, 2022 20:55

engine-flutter-autoroll mentioned this pull request May 9, 2022

Roll Flutter from b305eb633780 to 16f3feaf0780 (38 revisions) flutter/packages#1860

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 9, 2022

0ff0aff Expose controller for PaginatedDataTable (flutter/flutter#100005

d2a695e

)

engine-flutter-autoroll mentioned this pull request May 9, 2022

Roll Flutter from 4bed76757db7 to 16f3feaf0780 (31 revisions) flutter/plugins#5673

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request May 9, 2022

0ff0aff Expose controller for PaginatedDataTable (flutter/flutter#100005

ce09007

)

engine-flutter-autoroll mentioned this pull request May 9, 2022

Roll Flutter from b305eb633780 to 93cce92ea827 (39 revisions) flutter/packages#1861

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 9, 2022

0ff0aff Expose controller for PaginatedDataTable (flutter/flutter#100005

d6b8681

)

engine-flutter-autoroll mentioned this pull request May 9, 2022

Roll Flutter from 4bed76757db7 to 93cce92ea827 (32 revisions) flutter/plugins#5674

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request May 9, 2022

0ff0aff Expose controller for PaginatedDataTable (flutter/flutter#100005

bb2ede3

)

engine-flutter-autoroll mentioned this pull request May 9, 2022

Roll Flutter from 4c573207acc3 to 93cce92ea827 (6 revisions) flutter/packages#1862

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 9, 2022

0ff0aff Expose controller for PaginatedDataTable (flutter/flutter#100005

6cfb097

)

engine-flutter-autoroll mentioned this pull request May 10, 2022

Roll Flutter from 4bed76757db7 to df7111a848cb (42 revisions) flutter/plugins#5696

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request May 10, 2022

0ff0aff Expose controller for PaginatedDataTable (flutter/flutter#100005

36b36ae

)

Conversation

amanv8060 commented Mar 12, 2022

Pre-launch Checklist

Uh oh!

flutter-dashboard bot commented Mar 12, 2022

Uh oh!

amanv8060 commented Mar 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Piinks left a comment

Choose a reason for hiding this comment

Uh oh!

amanv8060 commented Mar 22, 2022

Uh oh!

amanv8060 commented Apr 2, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Piinks Apr 5, 2022

Choose a reason for hiding this comment

Uh oh!

amanv8060 Apr 5, 2022

Choose a reason for hiding this comment

Uh oh!

Piinks Apr 7, 2022

Choose a reason for hiding this comment

Uh oh!

amanv8060 Apr 7, 2022

Choose a reason for hiding this comment

Uh oh!

amanv8060 Apr 18, 2022

Choose a reason for hiding this comment

Uh oh!

Piinks Apr 19, 2022

Choose a reason for hiding this comment

Uh oh!

amanv8060 Apr 19, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Piinks left a comment

Choose a reason for hiding this comment

Uh oh!

Piinks commented Apr 27, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

amanv8060 commented Mar 13, 2022 •

edited

Loading