You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2022/02/10 19:39:59 UTC
[GitHub] [superset] corbinrobb opened a new pull request #18666: fix(sqllab): fix query results sorting
corbinrobb opened a new pull request #18666:
URL: https://github.com/apache/superset/pull/18666
### SUMMARY
- Adds/fixes sorting for results with more than 50 columns
- Adds third sort state that clears the sort and returns list to its initial order
- Adds a little more padding for the sorting icons because they were being truncated on some headers
---
### Sorting
In SQL Lab there is currently no sorting for data with more than 50 columns because `FIlterableTable` switches from using the react-virtualized `Table` to the react-virtualized `Grid`. [Grid](https://github.com/bvaughn/react-virtualized/blob/master/docs/Grid.md) isn't set up to have sorting the way that [Table](https://github.com/bvaughn/react-virtualized/blob/master/docs/Table.md) is.
The UI is the same as far as the user can tell so the sorting appears to stop working when there are too many columns.
I added listeners to the grid headers and added a `displayedList` state that gets sorted and used by the grid cells when rendering. I used the `SortIndicator` icon component and used the same `sortBy` and `sortDirection` state that the `Table` uses. They are never rendered at the same time so it didn't feel necessary to give the Grid its own separate states.
For the same reason I slightly changed the `renderTable` method to use the new `displayedList` state as well.
This allows the initial list to remain intact because it is now doing "out of place" sorting instead of "in place" sorting. This allowed me to add a third cleared sort state that reverts the sorting back to before it was sorted.
---
### Third Sort State (Cleared Sort State)
Added a third sorting state to FilterableTable that will return the results to its original order. As far as I know, this was not requested anywhere but I added it because I was in the area.
This is done by having an initial list and a displayed list and only mutating the displayed list when sorting. Then we just set the list back to its initial state when we have already sorted by descending and ascending.
The logic and flow is simple enough
The current sorting flow is:
- Original list order
- Click -> Ascending list order
- Click -> Descending list order
- Click -> Ascending list order
This PR will change it to:
- Original list order
- Click -> Ascending list order
- Click -> Descending list order
- Click -> Original list order
**Logic:**
```javascript
sort({
sortBy,
sortDirection,
}: {
sortBy: string;
sortDirection: SortDirectionType;
}) {
this.setState(
({
sortBy: prevSortBy,
sortDirection: prevSortDirection,
displayedList: prevDisplayedList,
}) => {
const shouldClearSort =
prevSortDirection === SortDirection.DESC && prevSortBy === sortBy;
if (shouldClearSort)
return {
sortBy: undefined,
sortDirection: undefined,
displayedList: [...this.list],
};
return {
sortBy,
sortDirection,
displayedList: prevDisplayedList.sort(
this.sortResults(sortBy, sortDirection === SortDirection.DESC),
),
};
},
);
}
```
---
### Extra Padding
Increases the constant for the default padding on cells from `40` to `50`
The sorting icon was being truncated on some cells so I added a little more padded to fix it.
### Screenshots
### BEFORE
**Example of clicking on headers in table with >50 columns**
**Example of sorting on tables <50 columns**
**Examples of headers being truncated after triggering sorting**
### AFTER
**Example of clicking on headers in table with >50 columns**
**Examples of headers not being truncated after triggering sorting**
### TESTING INSTRUCTIONS
<!--- Required! What steps can be taken to manually verify the changes? -->
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [x] Has associated issue: #16261
- [ ] Required feature flags:
- [x] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] github-actions[bot] commented on pull request #18666: fix(sqllab): fix query results sorting
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #18666:
URL: https://github.com/apache/superset/pull/18666#issuecomment-1035694893
@yousoph Ephemeral environment spinning up at http://34.219.227.216:8080. Credentials are `admin`/`admin`. Please allow several minutes for bootstrapping and startup.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] AAfghahi commented on pull request #18666: fix(sqllab): fix query results sorting
Posted by GitBox <gi...@apache.org>.
AAfghahi commented on pull request #18666:
URL: https://github.com/apache/superset/pull/18666#issuecomment-1035633186
This looks really good, I had some basic questions and one suggestion.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] corbinrobb commented on a change in pull request #18666: fix(sqllab): fix query results sorting
Posted by GitBox <gi...@apache.org>.
corbinrobb commented on a change in pull request #18666:
URL: https://github.com/apache/superset/pull/18666#discussion_r804341540
##########
File path: superset-frontend/src/components/FilterableTable/FilterableTable.tsx
##########
@@ -295,7 +296,30 @@ export default class FilterableTable extends PureComponent<
sortBy: string;
sortDirection: SortDirectionType;
}) {
- this.setState({ sortBy, sortDirection });
+ this.setState(
Review comment:
We probably can. What do you mean by sort direction though? I am only seeing the `sortDirection` param for the sort method and the `prevSortDirection` param in the setState callback. I am not sure how to turn either into its own function though
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] corbinrobb commented on a change in pull request #18666: fix(sqllab): fix query results sorting
Posted by GitBox <gi...@apache.org>.
corbinrobb commented on a change in pull request #18666:
URL: https://github.com/apache/superset/pull/18666#discussion_r804312392
##########
File path: superset-frontend/src/components/FilterableTable/FilterableTable.tsx
##########
@@ -114,8 +114,9 @@ interface FilterableTableProps {
interface FilterableTableState {
sortBy?: string;
- sortDirection: SortDirectionType;
+ sortDirection?: SortDirectionType;
Review comment:
It's because of the third unsorted state that I added. Before it was being set with an initial value of "DESC" but it would be unsorted because sortBy starts as undefined. On the first click it would be set to "ASC" and sortBy gets set to that columns name. So now I have it set as undefined/optional initially and then clicking will set it to "ASC" then "DESC" then back to undefined.
I don't really like using undefined like that and I prefer to use null but the in the react-virtualized typing for SortIndicator and Table they have sortDirection set as optional. So if i used null I would have to do a fallback to undefined anyways because of their typing
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] github-actions[bot] commented on pull request #18666: fix(sqllab): fix query results sorting
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #18666:
URL: https://github.com/apache/superset/pull/18666#issuecomment-1035621162
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] yousoph commented on pull request #18666: fix(sqllab): fix query results sorting
Posted by GitBox <gi...@apache.org>.
yousoph commented on pull request #18666:
URL: https://github.com/apache/superset/pull/18666#issuecomment-1035694074
/testenv up
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] yousoph commented on pull request #18666: fix(sqllab): fix query results sorting
Posted by GitBox <gi...@apache.org>.
yousoph commented on pull request #18666:
URL: https://github.com/apache/superset/pull/18666#issuecomment-1035620741
/testenv up
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] corbinrobb commented on a change in pull request #18666: fix(sqllab): fix query results sorting
Posted by GitBox <gi...@apache.org>.
corbinrobb commented on a change in pull request #18666:
URL: https://github.com/apache/superset/pull/18666#discussion_r804315739
##########
File path: superset-frontend/src/components/FilterableTable/FilterableTable.tsx
##########
@@ -175,8 +176,8 @@ export default class FilterableTable extends PureComponent<
this.totalTableHeight = props.height;
this.state = {
- sortDirection: SortDirection.ASC,
fitted: false,
+ displayedList: [...this.list],
Review comment:
Should be all of it. The data gets formatted and set to the `list` variable in the constructor
```javascript
constructor(props: FilterableTableProps) {
super(props);
this.list = this.formatTableData(props.data);
```
That's all the data that needs to be sorted from what I can tell. Lemme know if you think I missed something though
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] codecov[bot] edited a comment on pull request #18666: fix(sqllab): fix query results sorting
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #18666:
URL: https://github.com/apache/superset/pull/18666#issuecomment-1035643003
# [Codecov](https://codecov.io/gh/apache/superset/pull/18666?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#18666](https://codecov.io/gh/apache/superset/pull/18666?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (63ba054) into [master](https://codecov.io/gh/apache/superset/commit/f03b4dbedb29087c6958804aa803b5fb3200c7bb?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (f03b4db) will **decrease** coverage by `0.00%`.
> The diff coverage is `64.28%`.
> :exclamation: Current head 63ba054 differs from pull request most recent head dda6bbf. Consider uploading reports for the commit dda6bbf to get more accurate results
[](https://codecov.io/gh/apache/superset/pull/18666?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #18666 +/- ##
==========================================
- Coverage 66.29% 66.28% -0.01%
==========================================
Files 1603 1603
Lines 62744 62750 +6
Branches 6320 6325 +5
==========================================
Hits 41593 41593
- Misses 19499 19505 +6
Partials 1652 1652
```
| Flag | Coverage Δ | |
|---|---|---|
| javascript | `51.27% <64.28%> (-0.01%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/18666?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...src/components/FilterableTable/FilterableTable.tsx](https://codecov.io/gh/apache/superset/pull/18666/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvRmlsdGVyYWJsZVRhYmxlL0ZpbHRlcmFibGVUYWJsZS50c3g=) | `70.85% <64.28%> (-1.17%)` | :arrow_down: |
| [...hart-echarts/src/MixedTimeseries/transformProps.ts](https://codecov.io/gh/apache/superset/pull/18666/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9wbHVnaW4tY2hhcnQtZWNoYXJ0cy9zcmMvTWl4ZWRUaW1lc2VyaWVzL3RyYW5zZm9ybVByb3BzLnRz) | `0.00% <0.00%> (ø)` | |
| [.../src/explore/components/controls/SliderControl.jsx](https://codecov.io/gh/apache/superset/pull/18666/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9jb250cm9scy9TbGlkZXJDb250cm9sLmpzeA==) | | |
| [.../src/explore/components/controls/SliderControl.tsx](https://codecov.io/gh/apache/superset/pull/18666/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9jb250cm9scy9TbGlkZXJDb250cm9sLnRzeA==) | `0.00% <0.00%> (ø)` | |
| [...lugin-chart-echarts/src/Timeseries/transformers.ts](https://codecov.io/gh/apache/superset/pull/18666/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9wbHVnaW4tY2hhcnQtZWNoYXJ0cy9zcmMvVGltZXNlcmllcy90cmFuc2Zvcm1lcnMudHM=) | `51.23% <0.00%> (+0.40%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/18666?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/18666?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [f03b4db...dda6bbf](https://codecov.io/gh/apache/superset/pull/18666?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] codecov[bot] edited a comment on pull request #18666: fix(sqllab): fix query results sorting
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #18666:
URL: https://github.com/apache/superset/pull/18666#issuecomment-1035643003
# [Codecov](https://codecov.io/gh/apache/superset/pull/18666?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#18666](https://codecov.io/gh/apache/superset/pull/18666?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (dda6bbf) into [master](https://codecov.io/gh/apache/superset/commit/f03b4dbedb29087c6958804aa803b5fb3200c7bb?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (f03b4db) will **decrease** coverage by `0.04%`.
> The diff coverage is `64.28%`.
[](https://codecov.io/gh/apache/superset/pull/18666?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #18666 +/- ##
==========================================
- Coverage 66.29% 66.24% -0.05%
==========================================
Files 1603 1604 +1
Lines 62744 62787 +43
Branches 6320 6336 +16
==========================================
- Hits 41593 41591 -2
- Misses 19499 19545 +46
+ Partials 1652 1651 -1
```
| Flag | Coverage Δ | |
|---|---|---|
| javascript | `51.21% <64.28%> (-0.08%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/18666?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...src/components/FilterableTable/FilterableTable.tsx](https://codecov.io/gh/apache/superset/pull/18666/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvRmlsdGVyYWJsZVRhYmxlL0ZpbHRlcmFibGVUYWJsZS50c3g=) | `71.42% <64.28%> (-0.60%)` | :arrow_down: |
| [...frontend/src/SqlLab/components/SouthPane/index.tsx](https://codecov.io/gh/apache/superset/pull/18666/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1NvdXRoUGFuZS9pbmRleC50c3g=) | `63.33% <0.00%> (ø)` | |
| [...rontend/src/components/AsyncEsmComponent/index.tsx](https://codecov.io/gh/apache/superset/pull/18666/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvQXN5bmNFc21Db21wb25lbnQvaW5kZXgudHN4) | `96.29% <0.00%> (ø)` | |
| [...ontend/src/SqlLab/components/QuerySearch/index.tsx](https://codecov.io/gh/apache/superset/pull/18666/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1F1ZXJ5U2VhcmNoL2luZGV4LnRzeA==) | `73.07% <0.00%> (ø)` | |
| [...ntend/src/SqlLab/components/QueryHistory/index.tsx](https://codecov.io/gh/apache/superset/pull/18666/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1F1ZXJ5SGlzdG9yeS9pbmRleC50c3g=) | `50.00% <0.00%> (ø)` | |
| [.../src/components/Select/WindowedSelect/windowed.tsx](https://codecov.io/gh/apache/superset/pull/18666/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvU2VsZWN0L1dpbmRvd2VkU2VsZWN0L3dpbmRvd2VkLnRzeA==) | `70.00% <0.00%> (ø)` | |
| [...legacy-preset-chart-deckgl/src/DeckGLContainer.jsx](https://codecov.io/gh/apache/superset/pull/18666/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9sZWdhY3ktcHJlc2V0LWNoYXJ0LWRlY2tnbC9zcmMvRGVja0dMQ29udGFpbmVyLmpzeA==) | `0.00% <0.00%> (ø)` | |
| [...hart-echarts/src/MixedTimeseries/transformProps.ts](https://codecov.io/gh/apache/superset/pull/18666/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9wbHVnaW4tY2hhcnQtZWNoYXJ0cy9zcmMvTWl4ZWRUaW1lc2VyaWVzL3RyYW5zZm9ybVByb3BzLnRz) | `0.00% <0.00%> (ø)` | |
| [...rontend/src/SqlLab/components/QueryTable/index.jsx](https://codecov.io/gh/apache/superset/pull/18666/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1F1ZXJ5VGFibGUvaW5kZXguanN4) | | |
| [...src/dashboard/components/DeleteComponentButton.jsx](https://codecov.io/gh/apache/superset/pull/18666/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL0RlbGV0ZUNvbXBvbmVudEJ1dHRvbi5qc3g=) | | |
| ... and [8 more](https://codecov.io/gh/apache/superset/pull/18666/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/18666?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/18666?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [f03b4db...dda6bbf](https://codecov.io/gh/apache/superset/pull/18666?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] github-actions[bot] commented on pull request #18666: fix(sqllab): fix query results sorting
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #18666:
URL: https://github.com/apache/superset/pull/18666#issuecomment-1039596745
Ephemeral environment shutdown and build artifacts deleted.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] eschutho merged pull request #18666: fix(sqllab): fix query results sorting
Posted by GitBox <gi...@apache.org>.
eschutho merged pull request #18666:
URL: https://github.com/apache/superset/pull/18666
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] AAfghahi commented on a change in pull request #18666: fix(sqllab): fix query results sorting
Posted by GitBox <gi...@apache.org>.
AAfghahi commented on a change in pull request #18666:
URL: https://github.com/apache/superset/pull/18666#discussion_r804227515
##########
File path: superset-frontend/src/components/FilterableTable/FilterableTable.tsx
##########
@@ -295,7 +296,30 @@ export default class FilterableTable extends PureComponent<
sortBy: string;
sortDirection: SortDirectionType;
}) {
- this.setState({ sortBy, sortDirection });
+ this.setState(
Review comment:
this is a lot to put in a setState, could we make the sort direction into its own function and pass it in here?
##########
File path: superset-frontend/src/components/FilterableTable/FilterableTable.tsx
##########
@@ -175,8 +176,8 @@ export default class FilterableTable extends PureComponent<
this.totalTableHeight = props.height;
this.state = {
- sortDirection: SortDirection.ASC,
fitted: false,
+ displayedList: [...this.list],
Review comment:
is this the entirety of the data?
##########
File path: superset-frontend/src/components/FilterableTable/FilterableTable.tsx
##########
@@ -114,8 +114,9 @@ interface FilterableTableProps {
interface FilterableTableState {
sortBy?: string;
- sortDirection: SortDirectionType;
+ sortDirection?: SortDirectionType;
Review comment:
why is this optional?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] eschutho merged pull request #18666: fix(sqllab): fix query results sorting
Posted by GitBox <gi...@apache.org>.
eschutho merged pull request #18666:
URL: https://github.com/apache/superset/pull/18666
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] codecov[bot] commented on pull request #18666: fix(sqllab): fix query results sorting
Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on pull request #18666:
URL: https://github.com/apache/superset/pull/18666#issuecomment-1035643003
# [Codecov](https://codecov.io/gh/apache/superset/pull/18666?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#18666](https://codecov.io/gh/apache/superset/pull/18666?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (63ba054) into [master](https://codecov.io/gh/apache/superset/commit/f03b4dbedb29087c6958804aa803b5fb3200c7bb?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (f03b4db) will **decrease** coverage by `0.00%`.
> The diff coverage is `64.28%`.
[](https://codecov.io/gh/apache/superset/pull/18666?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #18666 +/- ##
==========================================
- Coverage 66.29% 66.28% -0.01%
==========================================
Files 1603 1603
Lines 62744 62750 +6
Branches 6320 6325 +5
==========================================
Hits 41593 41593
- Misses 19499 19505 +6
Partials 1652 1652
```
| Flag | Coverage Δ | |
|---|---|---|
| javascript | `51.27% <64.28%> (-0.01%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/18666?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...src/components/FilterableTable/FilterableTable.tsx](https://codecov.io/gh/apache/superset/pull/18666/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvRmlsdGVyYWJsZVRhYmxlL0ZpbHRlcmFibGVUYWJsZS50c3g=) | `70.85% <64.28%> (-1.17%)` | :arrow_down: |
| [...hart-echarts/src/MixedTimeseries/transformProps.ts](https://codecov.io/gh/apache/superset/pull/18666/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9wbHVnaW4tY2hhcnQtZWNoYXJ0cy9zcmMvTWl4ZWRUaW1lc2VyaWVzL3RyYW5zZm9ybVByb3BzLnRz) | `0.00% <0.00%> (ø)` | |
| [.../src/explore/components/controls/SliderControl.jsx](https://codecov.io/gh/apache/superset/pull/18666/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9jb250cm9scy9TbGlkZXJDb250cm9sLmpzeA==) | | |
| [.../src/explore/components/controls/SliderControl.tsx](https://codecov.io/gh/apache/superset/pull/18666/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9jb250cm9scy9TbGlkZXJDb250cm9sLnRzeA==) | `0.00% <0.00%> (ø)` | |
| [...lugin-chart-echarts/src/Timeseries/transformers.ts](https://codecov.io/gh/apache/superset/pull/18666/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9wbHVnaW4tY2hhcnQtZWNoYXJ0cy9zcmMvVGltZXNlcmllcy90cmFuc2Zvcm1lcnMudHM=) | `51.23% <0.00%> (+0.40%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/18666?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/18666?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [f03b4db...63ba054](https://codecov.io/gh/apache/superset/pull/18666?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] github-actions[bot] commented on pull request #18666: fix(sqllab): fix query results sorting
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #18666:
URL: https://github.com/apache/superset/pull/18666#issuecomment-1039596745
Ephemeral environment shutdown and build artifacts deleted.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org