-
Notifications
You must be signed in to change notification settings - Fork 28.9k
Fix sort indicator for DataTables #62795
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
Fix sort indicator for DataTables #62795
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
c8c3aec
to
e199b52
Compare
@googlebot I signed it! |
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. Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
Change looks good but it does need a test. In theory you would think that this change should have broken one, but I guess our test coverage was insufficient in this area. |
@Hixie Ok, I will have a look, but it is going to be a while as this will be my first UI test… 🙈 |
As per material spec ascending order should be shown via an upward arrow This commit changes the displayed arrows accordingly.
e199b52
to
2ed63ed
Compare
@Hixie Added a unit test as requested :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice job. Thanks for the contribution. I just had a minor nit on the name of the test, but otherwise it LGTM. Also you should update the description in the PR to include that you added a test to verify the arrow was pointing in the correct orientation.
We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. ℹ️ Googlers: Go here for more info. |
Thanks :) |
7d2ef62
to
4a1d77d
Compare
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
Ah, I didn't look at the context. Perhaps the original author had a different opinion, in which case you were probably right to follow what was there. At any rate, let's go with this for now. Once the tests pass I will get it merged. Thx. |
@darrenaustin Right now everything is green :) |
* Fix sort indicator for DataTables As per material spec ascending order should be shown via an upward arrow This commit changes the displayed arrows accordingly. * Test sort indicator orientation in DataTable
Description
As per material spec ascending order should be shown via an upward arrow
This PR changes the displayed arrows accordingly. for DataTables as well as PaginatedDataTables
In theory adding a single
!
to inverse theascending
value would have had the same effect as my suggested change. My change tries to keep the code concise and intuitive.I kept the original formatting to make the changes easier to review, although
dartfmt
would suggest quite some "whitespace" changes.Related Issues
Tests
A test to check that the arrow indicator has the proper orientation was added.
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.///
).flutter analyze --flutter-repo
) does not report any problems on my PR.Breaking Change
Did any tests fail when you ran them? Please read Handling breaking changes.