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.

Sign up for GitHub

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

Jump to bottom

Material DataTable: added support of setting table row border thickness #49692

Merged
merged 6 commits into from
Feb 24, 2020
Merged

Material DataTable: added support of setting table row border thickness #49692

merged 6 commits into from
Feb 24, 2020

Conversation

dratushnyy
Copy link
Contributor

@dratushnyy dratushnyy commented Jan 29, 2020 •
edited
Loading

Description

At the current moment, there is no option in the DataTable widget to setup row thickness since the value of it is hardcoded and set to 1.0 (please refer to the related issue for more detailed description).
This pull request introduced changes fixing the issue, namely, try to use Theme.of(context).dividerTheme.thickness and if it is null then use a default value of 1.0 which was moved to a named constant inside DataTable class to increase code readability.

Related Issues

Fixes #49691

Tests

I added the following tests:
DataTable set border width test into packages\flutter\test\material\data_table_test.dart

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I signed the CLA.
  • I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I updated/added relevant documentation (doc comments with ///).
  • All existing and new tests are passing.
  • The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Did any tests fail when you ran them? Please read Handling breaking changes.

  • No, no existing tests failed, so this is not a breaking change.
  • Yes, this is a breaking change. If not, delete the remainder of this section.
    • I wrote a design doc: https://flutter.dev/go/template Replace this with a link to your design doc's short link
    • I got input from the developer relations team, specifically from: Replace with the names of who gave advice
    • I wrote a migration guide: Replace with a link to your migration guide

@dratushnyy
Material DataTable: added support of setting table row border thickness
4443df7 
using Theme.dividerTheme.thickness or, if the theme contains null-value
- the value of DataTable._defaultBodrerThickness constant
which was introduced to provide more meaningful value instead
of using nameless double.
@fluttergithubbot fluttergithubbot added f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. labels Jan 29, 2020
@dratushnyy dratushnyy requested review from goderbauer, shihaohong and a14n and removed request for goderbauer and shihaohong January 29, 2020 11:23
@dratushnyy dratushnyy mentioned this pull request Feb 6, 2020
Closed
Piinks
Piinks reviewed Feb 11, 2020
View reviewed changes
Copy link
Contributor

@Piinks Piinks left a comment

Choose a reason for hiding this comment

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

Hi @dratushnyy welcome! 🎉 Thank you for the contribution and for writing a test!
I think this change looks ok, although @HansMuller may have some thoughts here. Particularly regarding www.flutter.dev/go/material-theme-system-updates

packages/flutter/lib/src/material/data_table.dart Outdated Show resolved Hide resolved
@HansMuller
Copy link
Contributor

For now, it would safer to just add DataTable.dividerThickness, and not to defer to the dividerTheme. As Kate mentioned, we're moving toward a theme system where there's an overall theme with a small number of properties, and component specific themes.

@dratushnyy
Material DataTable: border thickess moved to DataTable
cc56bc5 
constructor parameter instead of using ThemeData during to
upcoming (and possibly breaking) changes to Material Theme
System.
@dratushnyy
Copy link
Contributor Author

@Piinks, @HansMuller Thank you for your comments, I updated the pull-request accordingly

HansMuller
HansMuller reviewed Feb 12, 2020
View reviewed changes
Copy link
Contributor

@HansMuller HansMuller left a comment

Choose a reason for hiding this comment

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

This is looking good, just some small stuff.

packages/flutter/lib/src/material/data_table.dart Outdated Show resolved Hide resolved
packages/flutter/lib/src/material/data_table.dart Outdated Show resolved Hide resolved
packages/flutter/lib/src/material/data_table.dart Outdated Show resolved Hide resolved
packages/flutter/lib/src/material/data_table.dart Outdated Show resolved Hide resolved
packages/flutter/test/material/data_table_test.dart Outdated Show resolved Hide resolved
packages/flutter/test/material/data_table_test.dart Outdated Show resolved Hide resolved
packages/flutter/test/material/data_table_test.dart Outdated Show resolved Hide resolved
packages/flutter/test/material/data_table_test.dart Outdated Show resolved Hide resolved
@dratushnyy
Material DataTabel: renamed border thickness constructor parameter and
be019e0 
field from 'borderThickness' to 'dividerThickness'.
Refactored related unit tests: simplify the creation of test data
table
Piinks
Piinks approved these changes Feb 13, 2020
View reviewed changes
Copy link
Contributor

@Piinks Piinks left a comment

Choose a reason for hiding this comment

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

This LGTM @dratushnyy, with one tiny nit below. Thank you for the contribution.
It also looks like your branch contains a flake that is causing the customer_* checks to fail. This is unrelated to your change, and should be resolved if you update your branch with changes from master. :)

packages/flutter/lib/src/material/data_table.dart Show resolved Hide resolved
@dratushnyy @Piinks
Reformatted documentation strings for "dividerThickness".
f5c756e 
Co-Authored-By: Kate Lovett <katelovett@google.com>
@HansMuller
Copy link
Contributor

Looks like a trival trailing whitespace error needs to be fixed:

packages/flutter/lib/src/material/data_table.dart:473: trailing U+0020 space character

@dratushnyy
Material Datatable: removed trailing space in documentaion
224c15f 
strings for `dividerThinkess` property
@dratushnyy
Copy link
Contributor Author

@HansMuller fixed. Sorry for the formatting issues.

@Piinks
Copy link
Contributor

Piinks commented Feb 21, 2020

It looks like this branch still needs to be updated to resolve the failing checks. :)

@Piinks
Copy link
Contributor

Piinks commented Feb 24, 2020

Thanks for the update! :)

@fluttergithubbot fluttergithubbot merged commit a70e4ae into flutter:master Feb 24, 2020
@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.
Reviewers

@HansMuller HansMuller HansMuller left review comments

@Piinks Piinks Piinks approved these changes

@goderbauer goderbauer Awaiting requested review from goderbauer

@a14n a14n Awaiting requested review from a14n

Assignees
No one assigned
Labels
f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

It is impossible to set thickness for rows in DataTable widget
5 participants
@dratushnyy @HansMuller @Piinks @googlebot @fluttergithubbot