Skip to content

[EuiDataGrid] Inverse handling of boolean sorting#4561

Merged
chandlerprall merged 21 commits intoelastic:masterfrom
anuragxxd:Dev-DatePicker
Feb 24, 2021
Merged

[EuiDataGrid] Inverse handling of boolean sorting#4561
chandlerprall merged 21 commits intoelastic:masterfrom
anuragxxd:Dev-DatePicker

Conversation

@anuragxxd
Copy link
Contributor

@anuragxxd anuragxxd commented Feb 21, 2021

Summary

Solved:- EuiDataGrid should switch handling of Boolean sorting. When there's ascending sorting, false values should be first, and the sorting text should be Sort False-True.
#4526

Checklist

- [ ] Check against all themes for compatibility in both light and dark modes
- [ ] Checked in mobile
- [ ] Checked in Chrome, Safari, Edge, and Firefox
- [ ] Props have proper autodocs and playground toggles
- [ ] Added documentation
- [ ] Checked Code Sandbox works for the any docs examples
- [ ] Added or updated jest tests

  • Checked for breaking changes and labeled appropriately
    - [ ] Checked for accessibility including keyboard-only and screenreader modes
  • A changelog entry exists and is marked appropriately

@kibanamachine
Copy link

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@anuragxxd anuragxxd closed this Feb 21, 2021
@anuragxxd anuragxxd changed the title Dev date picker [EUIDatePicker] Inverse handling of boolean sorting Feb 21, 2021
@anuragxxd anuragxxd reopened this Feb 21, 2021
@cchaos cchaos changed the title [EUIDatePicker] Inverse handling of boolean sorting [EuiDataGrid] Inverse handling of boolean sorting Feb 21, 2021
@cchaos cchaos requested a review from chandlerprall February 21, 2021 16:03
Co-authored-by: Akash Gupta <akashgp9@gmail.com>
Copy link
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

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

Thanks for picking this one up too, couple of comments:

Please revert the deletion of docs/bundle.min.js.map

This change inverts the text as needed but not the sorting logic which is located on lines 93-99. For testing, there is a boolean column on the _Data grid schemas and popovers
_ example

@anuragxxd
Copy link
Contributor Author

Done with the changes @chandlerprall

@chandlerprall
Copy link
Contributor

jenkins test this

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_4561/

Copy link
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

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

I've pushed an update to the changelog entry - we made a new EUI release and that causes git to place unmerged changelog items in the wrong section.

Tested these changes locally in the Data grid schemas and popovers example, will merge when CI passes.

@chandlerprall
Copy link
Contributor

jenkins test this

@chandlerprall
Copy link
Contributor

chandlerprall commented Feb 23, 2021

@git-anurag-hub Thanks again! One thing to note for future changes: please create a new branch from master for each PR, this avoids the noisy commit history and makes the changes in each PR easier to understand.

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_4561/

@chandlerprall
Copy link
Contributor

Error: Command failed: docker pull docker.elastic.co/eui/puppeteer:latest

jenkins test this

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_4561/

@anuragxxd
Copy link
Contributor Author

@git-anurag-hub Thanks again! One thing to note for future changes: please create a new branch from master for each PR, this avoids the noisy commit history and makes the changes in each PR easier to understand.

Yeah sorry for that will keep that in mind. :-\

@chandlerprall
Copy link
Contributor

CI timed out again; running tests locally to confirm everything is good, then merging

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants