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 2021/12/03 01:53:52 UTC
[GitHub] [superset] corbinrobb opened a new pull request #17638: [WIP] fix(select): select component sort functionality on certain options
corbinrobb opened a new pull request #17638:
URL: https://github.com/apache/superset/pull/17638
### SUMMARY
Working on fixing the issues with row limits, series limits, and other select options being sorted by their label instead of their number value. These are in the Explore view but the select component is used all over the place.
**TL;DR**
Some options are being sorted funky and there is other weirdness happening. I have implemented a fix that will sort the numbers but there may be better ways to do it. I am currently enumerating over files where select is being used and I am working on a solution that will satisfy all of options that get passed while not unnecessarily bloating or increasing the complexity of the component or codebase.
### What's happening:
So far I have figured out that the defaultSortOperator from Select (`superset-frontend/src/components/Select/Select.tsx`) (pictured below) is sorting the options being passed in to by their label if the options have a label that is `typeof 'string'`. The problem is that the numbers that are being passed in with a string label and a number value ex: `{ label: '20', value: 20 }`.
When the `Select` is inside of a `SelectControl` (`superset-frontend/src/explore/components/controls/SelectControl.jsx`) like it is when it is being used for viz plugins, the numbers are always being passed in with a label thanks to formatting.
So the below function is comparing a.label to b.label and sorting the numbers by their labels because from what I have been able to find, there aren't any (many?) scenarios that numbers are being passed in without a label.
```javascript
const defaultSortComparator = (a: AntdLabeledValue, b: AntdLabeledValue) => {
if (typeof a.label === 'string' && typeof b.label === 'string') {
return a.label.localeCompare(b.label);
}
if (typeof a.value === 'string' && typeof b.value === 'string') {
return a.value.localeCompare(b.value);
}
return (a.value as number) - (b.value as number);
};
```
## Discussion
If you are like me you are probably thinking, "Okay cool, then let's change that default operator to compare by value when the value is a number". The problem I found with this is that there is at least one other implementation of Select ( I currently only know of the Dataset selection in the Charts view) where label is a string with a title and the value is a number. So doing that changes those options to be sorted by their number value causing the options to appear out of order. I have not looked into what is going on there but I am assuming that the numbers serve a good purpose like an index for an API call to the backend.
### Current Fix:
The fix that has makes the most sense to me, with my current knowledge and understanding of what's happening, is to utilize the `propertyComparator` function right below `defaultSortComparator` as the default sort function used and add an optional property called `sortByProperty` to Select that's default value is `'label'` that I call and assign to sortOperator property on Select like so `sortComparator = propertyComparator(sortByProperty)`. This will have the default sorting be done by label and allow the option to pass in a string property value to sort by a specific property.
```javascript
export const propertyComparator =
(property: string) => (a: AntdLabeledValue, b: AntdLabeledValue) => {
if (typeof a[property] === 'string' && typeof b[property] === 'string') {
return a[property].localeCompare(b[property]);
}
return (a[property] as number) - (b[property] as number);
};
```
This works for the cases where we getting viz plugins like MixedTimeSeries as long as we add the sortBy value into the control configurations for the data visualizations as well as pass the prop down into Select after it is passed into SelectControl.
This also would allow us to remove the `defaultSortComparator` function and also remove where `propertyComparator` is being imported into some components and just pass in the property to Select as a string. I have had `defaultSortComparator` commented out for a while and haven't noticed anything but certainly doesn't mean something didn't break. I am planning to find every implementation of Select and check all the options being passed in. I will also manually test this.
No matter what ends up being the solution to this, I believe it is a must to add more tests to all of the current tests it has to verify that it works with numbers as expected.
## IMPORTANT
I am not sure about my solution and the more that I am looking into this, the more I am noticing other issues and getting more ideas. All of this directly involves the Select component and its sort functionality, so it feels appropriate to attempt to fix it or at the very least try to get an idea of what's happening so I can help somebody else do so. Also, I have not gone through and added this property to all the viz configurations that would use it yet because I am still researching.
- The number arrays that are being passed in are constant variables that are starting out sorted and then are being unsorted by the Select component once they get passed in. It might make sense to have the sorting be optional because not all options being rendered make sense when sorted. Like for instance Dates, and that brings me to the next thing.
- Dates are sent in sorted from smallest time to largest time and are then sorted by label and displayed unsorted. I am of the opinion that dates should go from smallest to largest but with these, we can't sort by either label or value because neither would work or make sense. This is what the label and value look like for some `{label: "Hour", value: "PT1H"}, {label: "Minute", value: "PT1M"}`. I feel these should not be sorted once they get into the Select or we could create a constant object that holds key:value pairs of the date option value and a corresponding size and then use that within the comparator function. But I don't like that idea because it would still involve having to know if the options are dates once they go in or we would have to write logic to check the options if they are dates and that feels, I dunno, gross and stupid? (I am very mature)
- There is another bug with the Select component where the sort order breaks upon selection if it has multiple selections enabled. On single mode it will close the component and the next time you open it it will be in order, but with multiple on it stays open after selecting an option and immediately changes the options to be out of order. This is weird and unexpected and it feels like it might have something to do with the sorting. I will attempt to fix this as well.
### BEFORE
<img width="469" alt="Screen Shot 2021-12-02 at 3 50 53 PM" src="https://user-images.githubusercontent.com/31329271/144515675-52def2fb-393f-4d18-9c54-3ffc1a9424da.png">
<img width="421" alt="Screen Shot 2021-12-02 at 4 00 41 PM" src="https://user-images.githubusercontent.com/31329271/144516685-a68c007c-8f81-49a2-91ae-bf2dada3cb1c.png">
### AFTER
<img width="421" alt="Screen Shot 2021-12-02 at 3 55 46 PM" src="https://user-images.githubusercontent.com/31329271/144516166-3ce0ea83-23ff-4e58-aa68-23859c00c797.png">
<img width="421" alt="Screen Shot 2021-12-02 at 3 59 27 PM" src="https://user-images.githubusercontent.com/31329271/144516555-048708dd-b843-4f1b-a6cf-4ab3ad394082.png">
### TESTING INSTRUCTIONS
Coming soon to a pull request near you.
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] 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 #17638: fix(select): select component sort functionality on certain options
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #17638:
URL: https://github.com/apache/superset/pull/17638#issuecomment-989117533
@AAfghahi Ephemeral environment spinning up at http://35.161.208.185: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] michael-s-molina edited a comment on pull request #17638: fix(select): select component sort functionality on certain options
Posted by GitBox <gi...@apache.org>.
michael-s-molina edited a comment on pull request #17638:
URL: https://github.com/apache/superset/pull/17638#issuecomment-989030451
@corbinrobb Thanks for the update and for fixing the initial sorting. I won't be able to test it but I agree with the changes.
@geido can you review it? If you think it's ok to merge it then I'm ok too.
--
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 #17638: fix(select): select component sort functionality on certain options
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #17638:
URL: https://github.com/apache/superset/pull/17638#issuecomment-988305524
# [Codecov](https://codecov.io/gh/apache/superset/pull/17638?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 [#17638](https://codecov.io/gh/apache/superset/pull/17638?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (4782473) into [master](https://codecov.io/gh/apache/superset/commit/b5d13d72f209132b6a334bf1978e96cbc06026d9?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (b5d13d7) will **increase** coverage by `0.21%`.
> The diff coverage is `50.00%`.
> :exclamation: Current head 4782473 differs from pull request most recent head 499ae7f. Consider uploading reports for the commit 499ae7f to get more accurate results
[](https://codecov.io/gh/apache/superset/pull/17638?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 #17638 +/- ##
==========================================
+ Coverage 68.55% 68.77% +0.21%
==========================================
Files 1602 1597 -5
Lines 65354 65214 -140
Branches 6994 6953 -41
==========================================
+ Hits 44806 44848 +42
+ Misses 18666 18479 -187
- Partials 1882 1887 +5
```
| Flag | Coverage Δ | |
|---|---|---|
| javascript | `57.45% <50.00%> (+0.34%)` | :arrow_up: |
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/17638?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...et-ui-chart-controls/src/shared-controls/index.tsx](https://codecov.io/gh/apache/superset/pull/17638/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-c3VwZXJzZXQtZnJvbnRlbmQvcGFja2FnZXMvc3VwZXJzZXQtdWktY2hhcnQtY29udHJvbHMvc3JjL3NoYXJlZC1jb250cm9scy9pbmRleC50c3g=) | `38.53% <0.00%> (-0.36%)` | :arrow_down: |
| [...ns/legacy-plugin-chart-heatmap/src/controlPanel.ts](https://codecov.io/gh/apache/superset/pull/17638/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-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9sZWdhY3ktcGx1Z2luLWNoYXJ0LWhlYXRtYXAvc3JjL2NvbnRyb2xQYW5lbC50cw==) | `80.00% <ø> (ø)` | |
| [...nd/plugins/plugin-chart-table/src/controlPanel.tsx](https://codecov.io/gh/apache/superset/pull/17638/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-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9wbHVnaW4tY2hhcnQtdGFibGUvc3JjL2NvbnRyb2xQYW5lbC50c3g=) | `21.34% <0.00%> (-0.50%)` | :arrow_down: |
| [...l/AdhocFilterEditPopoverSimpleTabContent/index.tsx](https://codecov.io/gh/apache/superset/pull/17638/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9jb250cm9scy9GaWx0ZXJDb250cm9sL0FkaG9jRmlsdGVyRWRpdFBvcG92ZXJTaW1wbGVUYWJDb250ZW50L2luZGV4LnRzeA==) | `67.15% <66.66%> (-0.26%)` | :arrow_down: |
| [.../src/explore/components/controls/SelectControl.jsx](https://codecov.io/gh/apache/superset/pull/17638/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9jb250cm9scy9TZWxlY3RDb250cm9sLmpzeA==) | `56.89% <66.66%> (-2.04%)` | :arrow_down: |
| [superset-frontend/src/components/Select/Select.tsx](https://codecov.io/gh/apache/superset/pull/17638/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvU2VsZWN0L1NlbGVjdC50c3g=) | `86.33% <100.00%> (ø)` | |
| [...packages/superset-ui-core/src/query/types/Query.ts](https://codecov.io/gh/apache/superset/pull/17638/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-c3VwZXJzZXQtZnJvbnRlbmQvcGFja2FnZXMvc3VwZXJzZXQtdWktY29yZS9zcmMvcXVlcnkvdHlwZXMvUXVlcnkudHM=) | `100.00% <0.00%> (ø)` | |
| [...ugins/legacy-plugin-chart-calendar/src/Calendar.js](https://codecov.io/gh/apache/superset/pull/17638/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-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9sZWdhY3ktcGx1Z2luLWNoYXJ0LWNhbGVuZGFyL3NyYy9DYWxlbmRhci5qcw==) | `0.00% <0.00%> (ø)` | |
| [...es/superset-ui-core/src/query/buildQueryContext.ts](https://codecov.io/gh/apache/superset/pull/17638/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-c3VwZXJzZXQtZnJvbnRlbmQvcGFja2FnZXMvc3VwZXJzZXQtdWktY29yZS9zcmMvcXVlcnkvYnVpbGRRdWVyeUNvbnRleHQudHM=) | `100.00% <0.00%> (ø)` | |
| ... and [18 more](https://codecov.io/gh/apache/superset/pull/17638/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/17638?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/17638?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 [b5d13d7...499ae7f](https://codecov.io/gh/apache/superset/pull/17638?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] corbinrobb commented on a change in pull request #17638: fix(select): [WIP] select component sort functionality on certain options
Posted by GitBox <gi...@apache.org>.
corbinrobb commented on a change in pull request #17638:
URL: https://github.com/apache/superset/pull/17638#discussion_r762497274
##########
File path: superset-frontend/src/components/Select/Select.tsx
##########
@@ -289,17 +303,20 @@ const Select = ({
pageSize = DEFAULT_PAGE_SIZE,
placeholder = t('Select ...'),
showSearch = true,
- sortComparator = defaultSortComparator,
+ sortByProperty = 'label',
+ sortComparator = propertyComparator(sortByProperty),
+ sortOptions = false,
value,
...props
}: SelectProps) => {
const isAsync = typeof options === 'function';
const isSingleMode = mode === 'single';
const shouldShowSearch = isAsync || allowNewOptions ? true : showSearch;
const initialOptions =
- options && Array.isArray(options) ? options : EMPTY_OPTIONS;
- const [selectOptions, setSelectOptions] =
- useState<OptionsType>(initialOptions);
+ options && Array.isArray(options) ? [...options] : EMPTY_OPTIONS;
+ const [selectOptions, setSelectOptions] = useState<OptionsType>(
+ sortOptions ? initialOptions.sort(sortComparator) : initialOptions,
Review comment:
You are right about the intention part but the sort operator is just the callback function that gets passed into the JS .sort() method. Im sure you have seen it a lot ex: `(a,b) => a - b` It was being passed in in a few different places and it doesn't do much because most are sent through propertyComparator. This is how all but one implementation looks
```javascript
sortComparator={propertyComparator('value')}
```
This has propertyComparator exported out of the same file along with Select to then be passed into it with a string. I added sortByProperty to avoid the need to do that because the function is already there and the only new information is the string telling it which property to sort by.
I think I understand what you were getting at though. I can condense my sortOptions boolean prop and sortByProperty string prop into one because the lack of the prop gives the same information.
```javascript
sortByProperty,
sortComparator = propertyComparator(sortByProperty || 'label'),
const [selectOptions, setSelectOptions] = useState<OptionsType>(
sortByProperty ? initialOptions.sort(sortComparator) : initialOptions,
);
```
Let me know what you think! I have another commit coming soon and I was planning on pushing this with it
--
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 merged pull request #17638: fix(select): select component sort functionality on certain options
Posted by GitBox <gi...@apache.org>.
AAfghahi merged pull request #17638:
URL: https://github.com/apache/superset/pull/17638
--
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 #17638: fix(select): [WIP] select component sort functionality on certain options
Posted by GitBox <gi...@apache.org>.
corbinrobb commented on a change in pull request #17638:
URL: https://github.com/apache/superset/pull/17638#discussion_r762135198
##########
File path: superset-frontend/src/components/Select/Select.tsx
##########
@@ -289,7 +303,9 @@ const Select = ({
pageSize = DEFAULT_PAGE_SIZE,
placeholder = t('Select ...'),
showSearch = true,
- sortComparator = defaultSortComparator,
+ sortByProperty = 'label',
+ sortComparator = propertyComparator(sortByProperty),
+ sortOptions = false,
Review comment:
Yeah it doesn't feel right. I can change that to true, I only have it like this because in the majority (I only have found one that isn't) of the places I found this component being used it is being passed a list that is in order. And a lot of them don't have a value that can easily be sorted upon. If I have it set as true as the default then I will need to go and set every static number list being passed in to sort by its value instead because it has been sorting them by default and that is what was unsorting them.
I figured that since most were like that then the default should reflect that but I may very well not be seeing all of them. I also can't think of a way that we can sort them appropriately without have to go manually add the prop. Do you have any thoughts on that? Would you like me to change it to true and add the property to every configuration where it should be sorted?
--
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 #17638: fix(select): select component sort functionality on certain options
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #17638:
URL: https://github.com/apache/superset/pull/17638#issuecomment-988305524
# [Codecov](https://codecov.io/gh/apache/superset/pull/17638?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 [#17638](https://codecov.io/gh/apache/superset/pull/17638?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (499ae7f) into [master](https://codecov.io/gh/apache/superset/commit/b5d13d72f209132b6a334bf1978e96cbc06026d9?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (b5d13d7) will **increase** coverage by `0.21%`.
> The diff coverage is `60.00%`.
[](https://codecov.io/gh/apache/superset/pull/17638?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 #17638 +/- ##
==========================================
+ Coverage 68.55% 68.77% +0.21%
==========================================
Files 1602 1597 -5
Lines 65354 65227 -127
Branches 6994 6956 -38
==========================================
+ Hits 44806 44857 +51
+ Misses 18666 18483 -183
- Partials 1882 1887 +5
```
| Flag | Coverage Δ | |
|---|---|---|
| javascript | `57.45% <60.00%> (+0.34%)` | :arrow_up: |
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/17638?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...et-ui-chart-controls/src/shared-controls/index.tsx](https://codecov.io/gh/apache/superset/pull/17638/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-c3VwZXJzZXQtZnJvbnRlbmQvcGFja2FnZXMvc3VwZXJzZXQtdWktY2hhcnQtY29udHJvbHMvc3JjL3NoYXJlZC1jb250cm9scy9pbmRleC50c3g=) | `38.53% <0.00%> (-0.36%)` | :arrow_down: |
| [...ns/legacy-plugin-chart-heatmap/src/controlPanel.ts](https://codecov.io/gh/apache/superset/pull/17638/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-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9sZWdhY3ktcGx1Z2luLWNoYXJ0LWhlYXRtYXAvc3JjL2NvbnRyb2xQYW5lbC50cw==) | `80.00% <ø> (ø)` | |
| [...nd/plugins/plugin-chart-table/src/controlPanel.tsx](https://codecov.io/gh/apache/superset/pull/17638/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-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9wbHVnaW4tY2hhcnQtdGFibGUvc3JjL2NvbnRyb2xQYW5lbC50c3g=) | `21.34% <0.00%> (-0.50%)` | :arrow_down: |
| [...l/AdhocFilterEditPopoverSimpleTabContent/index.tsx](https://codecov.io/gh/apache/superset/pull/17638/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9jb250cm9scy9GaWx0ZXJDb250cm9sL0FkaG9jRmlsdGVyRWRpdFBvcG92ZXJTaW1wbGVUYWJDb250ZW50L2luZGV4LnRzeA==) | `67.15% <66.66%> (-0.26%)` | :arrow_down: |
| [superset-frontend/src/components/Select/Select.tsx](https://codecov.io/gh/apache/superset/pull/17638/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvU2VsZWN0L1NlbGVjdC50c3g=) | `86.33% <100.00%> (ø)` | |
| [.../src/explore/components/controls/SelectControl.jsx](https://codecov.io/gh/apache/superset/pull/17638/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9jb250cm9scy9TZWxlY3RDb250cm9sLmpzeA==) | `60.34% <100.00%> (+1.41%)` | :arrow_up: |
| [...rset-ui-core/src/connection/SupersetClientClass.ts](https://codecov.io/gh/apache/superset/pull/17638/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-c3VwZXJzZXQtZnJvbnRlbmQvcGFja2FnZXMvc3VwZXJzZXQtdWktY29yZS9zcmMvY29ubmVjdGlvbi9TdXBlcnNldENsaWVudENsYXNzLnRz) | `90.76% <0.00%> (-7.54%)` | :arrow_down: |
| [...rc/explore/components/ExploreChartHeader/index.jsx](https://codecov.io/gh/apache/superset/pull/17638/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9FeHBsb3JlQ2hhcnRIZWFkZXIvaW5kZXguanN4) | `46.57% <0.00%> (-0.65%)` | :arrow_down: |
| [...packages/superset-ui-core/src/query/types/Query.ts](https://codecov.io/gh/apache/superset/pull/17638/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-c3VwZXJzZXQtZnJvbnRlbmQvcGFja2FnZXMvc3VwZXJzZXQtdWktY29yZS9zcmMvcXVlcnkvdHlwZXMvUXVlcnkudHM=) | `100.00% <0.00%> (ø)` | |
| [...ugins/legacy-plugin-chart-calendar/src/Calendar.js](https://codecov.io/gh/apache/superset/pull/17638/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-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9sZWdhY3ktcGx1Z2luLWNoYXJ0LWNhbGVuZGFyL3NyYy9DYWxlbmRhci5qcw==) | `0.00% <0.00%> (ø)` | |
| ... and [20 more](https://codecov.io/gh/apache/superset/pull/17638/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/17638?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/17638?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 [b5d13d7...499ae7f](https://codecov.io/gh/apache/superset/pull/17638?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] corbinrobb commented on pull request #17638: fix(select): [WIP] select component sort functionality on certain options
Posted by GitBox <gi...@apache.org>.
corbinrobb commented on pull request #17638:
URL: https://github.com/apache/superset/pull/17638#issuecomment-985163838
@hughhhh @michael-s-molina I was told that I should tag both of you. If you have any thoughts or suggestions I am very open to hearing them. I know that Michael wrote the logic and functions for the sorting in the PR to fix the order of select when selecting. So I imagine you would know if I am missing information or I am breaking something that I shouldn't
--
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 edited a comment on pull request #17638: fix(select): [WIP] select component sort functionality on certain options
Posted by GitBox <gi...@apache.org>.
corbinrobb edited a comment on pull request #17638:
URL: https://github.com/apache/superset/pull/17638#issuecomment-985676583
## Update 1
Okay big update here. As I was going through and adding all of the sortByProperty tags into the viz plugin configurations, I noticed that it was by far much more common for the options that are being passed into the Select component to be in it's intended order as it gets passed in.
After getting a much better feel for the component and what is going on in it, I came to the conclusion that passing in a new property for the configuration to define what property the options are getting sorted by was a backwards solution. The problem isn't the sorting, it was the assumption that the options were coming in sorted by one of their properties. Having the options come in and then sorting them by a property was assuming that this wasn't putting them out of order.
### Fix
The fix I have come up with that appears to work across the board for the numbers, dates, and every set of options that are passed into the Select, is to not sort by a property by default. It might seem a little weird but bear with me because I feel like it makes a lot of sense.
From what I could see main purpose of most of the sorting in the component was using it as a way to have the options go back to their original order once an option is deselected instead of having it sit at the top of the options. That makes sense but since the options were in a order that isn't easily replicated by comparing any of its values, I feel it made the most sense to create a dictionary or hash map of that original order and sort the values by that. Below are my functions were I implement this.
```javascript
const getInitialIndexes = (options: OptionsType) =>
options.reduce((a, c, i) => ({ ...a, [c.value]: i }), {});
const sortByInitialIndexes = (
options: OptionsType,
originals: { value?: number },
) => options.sort((a, b) => originals[a.value] - originals[b.value]);
```
What is going on here is the options are getting passed to `getInitialIndexes` and it is returning a hashmap that has a key:value pair of the options value and the index of the option in the array. This gives us the initial indexes of the options and allows us to grab them in O(1) time using a property that all the options have. With this, we can use those index values as the thing that we compare in our sort functions.
This happens in `sortByInitialIndexes` and is done by taking a param of the options array and an original indexes hash and then using the indexes as the comparison. I like the way that this works because it is always comparing numbers and it doesn't matter whether the values are strings or numbers. It is only concerned about the original indexes and does not need to try to decide what the best way sort the type of data.
I figured that since this hash needs to be created once and only gets read after that, it made sense to store it in a reference. We can then pass this ref into the sort function and we have the options back in their original order.
Here is an example where sort was being used in the handleTopOptions function.
```javascript
const sortedOptions = [
...sortByInitialIndexes(topOptions, optionsInitialOrder.current),
...sortByInitialIndexes(otherOptions, optionsInitialOrder.current),
];
if (!isEqual(sortedOptions, selectOptions)) {
setSelectOptions(sortedOptions);
}
```
Here is where the selected options are being put at the top and the unselected options are being put below them. They were both being sorted by `defaultSortComparator` before which was sorting them by label. Now with this we are putting the selected options at the top and then the rest of the options are just being sorted in the value that they were already in.
Here is where the same thing is happening when options are deselected
```javascript
if (options.length > 0) {
const sortedOriginal = sortByInitialIndexes(
selectOptions,
optionsInitialOrder.current,
);
setSelectOptions([...options, ...sortedOriginal]);
}
```
The same idea as the other but now the deselected option/options are being sorted to the order it was in before being selected.
This seems to work really well but there was a set of options that is in a lot of viz plugins that use the selection 'group by' and that is passed in unsorted and it made sense to add an option to have those be sorted by a certain value. So I added a boolean prop that lets the component know that the initial options should be sorted by this property. It then sorts the options by that property and sets the options to it while adding that initial sorted order into the hashmap.
```javascript
const [selectOptions, setSelectOptions] = useState<OptionsType>(
sortOptions ? initialOptions.sort(sortComparator) : initialOptions,
);
const optionsInitialOrder = useRef(getInitialIndexes(initialOptions));
```
This sets the initial options to be sorted and sets the `selectOptions` state to start at that initial value.
### Conclusion
So that's pretty much it, I still would like to add some tests and I also need to do some small changes that I noticed as I was writing this. As always, there might be something I am not seeing but this fix seems to work really well so far. Oh and I still left the defaultSortOperator commented out in there. I wanted to leave it as a reference for anybody who wants to help me decide if I should remove it or keep it around and still use it.
### Before
<img width="421" alt="Screen Shot 2021-12-03 at 9 23 52 AM" src="https://user-images.githubusercontent.com/31329271/144636960-ae5d857e-a069-403f-836d-27ba23906788.png">
<img width="421" alt="Screen Shot 2021-12-03 at 9 23 19 AM" src="https://user-images.githubusercontent.com/31329271/144636870-dd0112e8-bbc2-44b6-87d7-9bf1b338ce5d.png">
<img width="421" alt="Screen Shot 2021-12-03 at 9 36 03 AM" src="https://user-images.githubusercontent.com/31329271/144638843-f36ca5d0-e3d6-41fa-ba7f-cec329a25ea2.png">
<img width="421" alt="Screen Shot 2021-12-03 at 9 36 30 AM" src="https://user-images.githubusercontent.com/31329271/144638917-5ba9d26d-8cc4-416e-b48c-73590d2ce80c.png">
<img width="421" alt="Screen Shot 2021-12-03 at 9 37 09 AM" src="https://user-images.githubusercontent.com/31329271/144639008-4752b50c-bef2-486d-a17c-d9cdd4d8bc21.png">
<img width="421" alt="Screen Shot 2021-12-03 at 9 37 21 AM" src="https://user-images.githubusercontent.com/31329271/144639044-c852d554-a002-44cb-8267-5f4b86410082.png">
### After
<img width="421" alt="Screen Shot 2021-12-03 at 9 31 23 AM" src="https://user-images.githubusercontent.com/31329271/144638215-38675da3-b62a-4373-b680-92e66d4ca75b.png">
<img width="421" alt="Screen Shot 2021-12-03 at 9 30 04 AM" src="https://user-images.githubusercontent.com/31329271/144638023-80f18e2c-6008-466c-ab63-769c4fd0fe77.png">
<img width="421" alt="Screen Shot 2021-12-03 at 9 38 50 AM" src="https://user-images.githubusercontent.com/31329271/144639224-c2c4da5e-8690-49f2-b8d9-39ed9cf08f9d.png">
<img width="421" alt="Screen Shot 2021-12-03 at 9 39 37 AM" src="https://user-images.githubusercontent.com/31329271/144639352-50e24ab7-d835-484f-8a95-0ffad2dc3807.png">
<img width="421" alt="Screen Shot 2021-12-03 at 9 40 18 AM" src="https://user-images.githubusercontent.com/31329271/144639458-369cdf80-6dfe-4aab-82e6-b9d3a811915d.png">
--
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] michael-s-molina commented on pull request #17638: fix(select): select component sort functionality on certain options
Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on pull request #17638:
URL: https://github.com/apache/superset/pull/17638#issuecomment-989030451
@corbinrobb Thanks for the update and for fixing the initial sort. I won't be able to test it but I agree with the changes.
@geido can you review it? If you think it's ok to merge it then I'm ok too.
--
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 #17638: fix(select): select component sort functionality on certain options
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #17638:
URL: https://github.com/apache/superset/pull/17638#issuecomment-990022948
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] codecov[bot] commented on pull request #17638: fix(select): [WIP] select component sort functionality on certain options
Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on pull request #17638:
URL: https://github.com/apache/superset/pull/17638#issuecomment-988305524
# [Codecov](https://codecov.io/gh/apache/superset/pull/17638?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 [#17638](https://codecov.io/gh/apache/superset/pull/17638?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (87a8432) into [master](https://codecov.io/gh/apache/superset/commit/b5d13d72f209132b6a334bf1978e96cbc06026d9?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (b5d13d7) will **increase** coverage by `0.21%`.
> The diff coverage is `44.44%`.
[](https://codecov.io/gh/apache/superset/pull/17638?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 #17638 +/- ##
==========================================
+ Coverage 68.55% 68.77% +0.21%
==========================================
Files 1602 1597 -5
Lines 65354 65214 -140
Branches 6994 6953 -41
==========================================
+ Hits 44806 44848 +42
+ Misses 18666 18479 -187
- Partials 1882 1887 +5
```
| Flag | Coverage Δ | |
|---|---|---|
| javascript | `57.45% <44.44%> (+0.34%)` | :arrow_up: |
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/17638?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...et-ui-chart-controls/src/shared-controls/index.tsx](https://codecov.io/gh/apache/superset/pull/17638/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-c3VwZXJzZXQtZnJvbnRlbmQvcGFja2FnZXMvc3VwZXJzZXQtdWktY2hhcnQtY29udHJvbHMvc3JjL3NoYXJlZC1jb250cm9scy9pbmRleC50c3g=) | `38.53% <0.00%> (-0.36%)` | :arrow_down: |
| [...ns/legacy-plugin-chart-heatmap/src/controlPanel.ts](https://codecov.io/gh/apache/superset/pull/17638/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-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9sZWdhY3ktcGx1Z2luLWNoYXJ0LWhlYXRtYXAvc3JjL2NvbnRyb2xQYW5lbC50cw==) | `80.00% <ø> (ø)` | |
| [...nd/plugins/plugin-chart-table/src/controlPanel.tsx](https://codecov.io/gh/apache/superset/pull/17638/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-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9wbHVnaW4tY2hhcnQtdGFibGUvc3JjL2NvbnRyb2xQYW5lbC50c3g=) | `21.34% <0.00%> (-0.50%)` | :arrow_down: |
| [superset-frontend/src/components/Select/Select.tsx](https://codecov.io/gh/apache/superset/pull/17638/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvU2VsZWN0L1NlbGVjdC50c3g=) | `86.33% <ø> (ø)` | |
| [...l/AdhocFilterEditPopoverSimpleTabContent/index.tsx](https://codecov.io/gh/apache/superset/pull/17638/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9jb250cm9scy9GaWx0ZXJDb250cm9sL0FkaG9jRmlsdGVyRWRpdFBvcG92ZXJTaW1wbGVUYWJDb250ZW50L2luZGV4LnRzeA==) | `67.15% <66.66%> (-0.26%)` | :arrow_down: |
| [.../src/explore/components/controls/SelectControl.jsx](https://codecov.io/gh/apache/superset/pull/17638/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9jb250cm9scy9TZWxlY3RDb250cm9sLmpzeA==) | `56.89% <66.66%> (-2.04%)` | :arrow_down: |
| [...packages/superset-ui-core/src/query/types/Query.ts](https://codecov.io/gh/apache/superset/pull/17638/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-c3VwZXJzZXQtZnJvbnRlbmQvcGFja2FnZXMvc3VwZXJzZXQtdWktY29yZS9zcmMvcXVlcnkvdHlwZXMvUXVlcnkudHM=) | `100.00% <0.00%> (ø)` | |
| [...ugins/legacy-plugin-chart-calendar/src/Calendar.js](https://codecov.io/gh/apache/superset/pull/17638/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-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9sZWdhY3ktcGx1Z2luLWNoYXJ0LWNhbGVuZGFyL3NyYy9DYWxlbmRhci5qcw==) | `0.00% <0.00%> (ø)` | |
| [...es/superset-ui-core/src/query/buildQueryContext.ts](https://codecov.io/gh/apache/superset/pull/17638/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-c3VwZXJzZXQtZnJvbnRlbmQvcGFja2FnZXMvc3VwZXJzZXQtdWktY29yZS9zcmMvcXVlcnkvYnVpbGRRdWVyeUNvbnRleHQudHM=) | `100.00% <0.00%> (ø)` | |
| ... and [18 more](https://codecov.io/gh/apache/superset/pull/17638/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/17638?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/17638?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 [b5d13d7...87a8432](https://codecov.io/gh/apache/superset/pull/17638?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 #17638: fix(select): [WIP] select component sort functionality on certain options
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #17638:
URL: https://github.com/apache/superset/pull/17638#issuecomment-988305524
# [Codecov](https://codecov.io/gh/apache/superset/pull/17638?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 [#17638](https://codecov.io/gh/apache/superset/pull/17638?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (4782473) into [master](https://codecov.io/gh/apache/superset/commit/b5d13d72f209132b6a334bf1978e96cbc06026d9?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (b5d13d7) will **increase** coverage by `0.21%`.
> The diff coverage is `50.00%`.
[](https://codecov.io/gh/apache/superset/pull/17638?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 #17638 +/- ##
==========================================
+ Coverage 68.55% 68.77% +0.21%
==========================================
Files 1602 1597 -5
Lines 65354 65214 -140
Branches 6994 6953 -41
==========================================
+ Hits 44806 44848 +42
+ Misses 18666 18479 -187
- Partials 1882 1887 +5
```
| Flag | Coverage Δ | |
|---|---|---|
| javascript | `57.45% <50.00%> (+0.34%)` | :arrow_up: |
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/17638?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...et-ui-chart-controls/src/shared-controls/index.tsx](https://codecov.io/gh/apache/superset/pull/17638/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-c3VwZXJzZXQtZnJvbnRlbmQvcGFja2FnZXMvc3VwZXJzZXQtdWktY2hhcnQtY29udHJvbHMvc3JjL3NoYXJlZC1jb250cm9scy9pbmRleC50c3g=) | `38.53% <0.00%> (-0.36%)` | :arrow_down: |
| [...ns/legacy-plugin-chart-heatmap/src/controlPanel.ts](https://codecov.io/gh/apache/superset/pull/17638/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-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9sZWdhY3ktcGx1Z2luLWNoYXJ0LWhlYXRtYXAvc3JjL2NvbnRyb2xQYW5lbC50cw==) | `80.00% <ø> (ø)` | |
| [...nd/plugins/plugin-chart-table/src/controlPanel.tsx](https://codecov.io/gh/apache/superset/pull/17638/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-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9wbHVnaW4tY2hhcnQtdGFibGUvc3JjL2NvbnRyb2xQYW5lbC50c3g=) | `21.34% <0.00%> (-0.50%)` | :arrow_down: |
| [...l/AdhocFilterEditPopoverSimpleTabContent/index.tsx](https://codecov.io/gh/apache/superset/pull/17638/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9jb250cm9scy9GaWx0ZXJDb250cm9sL0FkaG9jRmlsdGVyRWRpdFBvcG92ZXJTaW1wbGVUYWJDb250ZW50L2luZGV4LnRzeA==) | `67.15% <66.66%> (-0.26%)` | :arrow_down: |
| [.../src/explore/components/controls/SelectControl.jsx](https://codecov.io/gh/apache/superset/pull/17638/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9jb250cm9scy9TZWxlY3RDb250cm9sLmpzeA==) | `56.89% <66.66%> (-2.04%)` | :arrow_down: |
| [superset-frontend/src/components/Select/Select.tsx](https://codecov.io/gh/apache/superset/pull/17638/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvU2VsZWN0L1NlbGVjdC50c3g=) | `86.33% <100.00%> (ø)` | |
| [...packages/superset-ui-core/src/query/types/Query.ts](https://codecov.io/gh/apache/superset/pull/17638/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-c3VwZXJzZXQtZnJvbnRlbmQvcGFja2FnZXMvc3VwZXJzZXQtdWktY29yZS9zcmMvcXVlcnkvdHlwZXMvUXVlcnkudHM=) | `100.00% <0.00%> (ø)` | |
| [...ugins/legacy-plugin-chart-calendar/src/Calendar.js](https://codecov.io/gh/apache/superset/pull/17638/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-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9sZWdhY3ktcGx1Z2luLWNoYXJ0LWNhbGVuZGFyL3NyYy9DYWxlbmRhci5qcw==) | `0.00% <0.00%> (ø)` | |
| [...es/superset-ui-core/src/query/buildQueryContext.ts](https://codecov.io/gh/apache/superset/pull/17638/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-c3VwZXJzZXQtZnJvbnRlbmQvcGFja2FnZXMvc3VwZXJzZXQtdWktY29yZS9zcmMvcXVlcnkvYnVpbGRRdWVyeUNvbnRleHQudHM=) | `100.00% <0.00%> (ø)` | |
| ... and [18 more](https://codecov.io/gh/apache/superset/pull/17638/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/17638?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/17638?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 [b5d13d7...4782473](https://codecov.io/gh/apache/superset/pull/17638?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] corbinrobb commented on pull request #17638: fix(select): [WIP] select component sort functionality on certain options
Posted by GitBox <gi...@apache.org>.
corbinrobb commented on pull request #17638:
URL: https://github.com/apache/superset/pull/17638#issuecomment-985676583
## Update 1
Okay big update here. As I was going through and adding all of the sortByProperty tags into the viz plugin configurations, I noticed that it was by far much more common for the options that are being passed into the Select component to be in it's intended order as it gets passed in.
After getting a much better feel for the component and what is going on in it, I came to the conclusion that passing in a new property for the configuration to define what property the options are getting sorted by was a backwards solution. The problem isn't the sorting, it was the assumption that the options were coming in sorted by one of their properties. Having the options come in and then sorting them by a property was assuming that this wasn't putting them out of order.
### Fix
The fix I have come up with that appears to work across the board for the numbers, dates, and every set of options that are passed into the Select, is to not sort by a property by default. It might seem a little weird but bear with me because I feel like it makes a lot of sense.
From what I could see main purpose of most of the sorting in the component was using it as a way to have the options go back to their original order once an option is deselected instead of having it sit at the top of the options. That makes sense but since the options were in a order that isn't easily replicated by comparing any of its values, I feel it made the most sense to create a dictionary or hash map of that original order and sort the values by that. Below are my functions were I implement this.
```javascript
const getInitialIndexes = (options: OptionsType) =>
options.reduce((a, c, i) => ({ ...a, [c.value]: i }), {});
const sortByInitialIndexes = (
options: OptionsType,
originals: { value?: number },
) => options.sort((a, b) => originals[a.value] - originals[b.value]);
```
What is going on here is the options are getting passed to `getInitialIndexes` and it is returning a hashmap that has a key:value pair of the options value and the index of the option in the array. This gives us the initial indexes of the options and allows us to grab them in O(1) time using a property that all the options have. With this, we can use those index values as the thing that we compare in our sort functions.
This happens in `sortByInitialIndexes` and is done by taking a param of the options array and an original indexes hash and then using the indexes as the comparison. I like the way that this works because it is always comparing numbers and it doesn't matter whether the values are strings or numbers. It is only concerned about the original indexes and does not need to try to decide what the best way sort the type of data.
I figured that since this hash needs to be created once and only gets read after that, it made sense to store it in a reference. We can then pass this ref into the sort function and we have the options back in their original order.
Here is an example where sort was being used in the handleTopOptions function.
```javascript
const sortedOptions = [
...sortByInitialIndexes(topOptions, optionsInitialOrder.current),
...sortByInitialIndexes(otherOptions, optionsInitialOrder.current),
];
if (!isEqual(sortedOptions, selectOptions)) {
setSelectOptions(sortedOptions);
}
```
Here is where the selected options are being put at the top and the unselected options are being put below them. They were both being sorted by `defaultSortComparator` before which was sorting them by label. Now with this we are putting the selected options at the top and then the rest of the options are just being sorted in the value that they were already in.
Here is where the same thing is happening when options are deselected
```javascript
if (options.length > 0) {
const sortedOriginal = sortByInitialIndexes(
selectOptions,
optionsInitialOrder.current,
);
setSelectOptions([...options, ...sortedOriginal]);
}
```
The same idea as the other but now the deselected option/options are being sorted to the order it was in before being selected.
This seems to work really well but there was a set of options that is in a lot of viz plugins that use the selection 'group by' and that is passed in unsorted and it made sense to add an option to have those be sorted by a certain value. So I added a boolean prop that lets the component know that the initial options should be sorted by this property. It then sorts the options by that property and sets the options to it while adding that initial sorted order into the hashmap.
```javascript
const [selectOptions, setSelectOptions] = useState<OptionsType>(
sortOptions ? initialOptions.sort(sortComparator) : initialOptions,
);
const optionsInitialOrder = useRef(getInitialIndexes(initialOptions));
```
This sets the initial options to be sorted and sets the `selectOptions` state to start at that initial value.
### Conclusion
So that's pretty much it, I still would like to add some tests and I also need to do some small changes that I noticed as I was writing this. As always, there might be something I am not seeing but this fix seems to work really well so far
### Before
<img width="421" alt="Screen Shot 2021-12-03 at 9 23 52 AM" src="https://user-images.githubusercontent.com/31329271/144636960-ae5d857e-a069-403f-836d-27ba23906788.png">
<img width="421" alt="Screen Shot 2021-12-03 at 9 23 19 AM" src="https://user-images.githubusercontent.com/31329271/144636870-dd0112e8-bbc2-44b6-87d7-9bf1b338ce5d.png">
<img width="421" alt="Screen Shot 2021-12-03 at 9 36 03 AM" src="https://user-images.githubusercontent.com/31329271/144638843-f36ca5d0-e3d6-41fa-ba7f-cec329a25ea2.png">
<img width="421" alt="Screen Shot 2021-12-03 at 9 36 30 AM" src="https://user-images.githubusercontent.com/31329271/144638917-5ba9d26d-8cc4-416e-b48c-73590d2ce80c.png">
<img width="421" alt="Screen Shot 2021-12-03 at 9 37 09 AM" src="https://user-images.githubusercontent.com/31329271/144639008-4752b50c-bef2-486d-a17c-d9cdd4d8bc21.png">
<img width="421" alt="Screen Shot 2021-12-03 at 9 37 21 AM" src="https://user-images.githubusercontent.com/31329271/144639044-c852d554-a002-44cb-8267-5f4b86410082.png">
### After
<img width="421" alt="Screen Shot 2021-12-03 at 9 31 23 AM" src="https://user-images.githubusercontent.com/31329271/144638215-38675da3-b62a-4373-b680-92e66d4ca75b.png">
<img width="421" alt="Screen Shot 2021-12-03 at 9 30 04 AM" src="https://user-images.githubusercontent.com/31329271/144638023-80f18e2c-6008-466c-ab63-769c4fd0fe77.png">
<img width="421" alt="Screen Shot 2021-12-03 at 9 38 50 AM" src="https://user-images.githubusercontent.com/31329271/144639224-c2c4da5e-8690-49f2-b8d9-39ed9cf08f9d.png">
<img width="421" alt="Screen Shot 2021-12-03 at 9 39 37 AM" src="https://user-images.githubusercontent.com/31329271/144639352-50e24ab7-d835-484f-8a95-0ffad2dc3807.png">
<img width="421" alt="Screen Shot 2021-12-03 at 9 40 18 AM" src="https://user-images.githubusercontent.com/31329271/144639458-369cdf80-6dfe-4aab-82e6-b9d3a811915d.png">
--
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] geido commented on a change in pull request #17638: fix(select): [WIP] select component sort functionality on certain options
Posted by GitBox <gi...@apache.org>.
geido commented on a change in pull request #17638:
URL: https://github.com/apache/superset/pull/17638#discussion_r762114548
##########
File path: superset-frontend/src/components/Select/Select.tsx
##########
@@ -289,7 +303,9 @@ const Select = ({
pageSize = DEFAULT_PAGE_SIZE,
placeholder = t('Select ...'),
showSearch = true,
- sortComparator = defaultSortComparator,
+ sortByProperty = 'label',
+ sortComparator = propertyComparator(sortByProperty),
+ sortOptions = false,
Review comment:
I am not too sure about having this set to false as sorting was by default before
--
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] michael-s-molina commented on pull request #17638: fix(select): [WIP] select component sort functionality on certain options
Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on pull request #17638:
URL: https://github.com/apache/superset/pull/17638#issuecomment-986795367
@corbinrobb Thank you for writing the PR with such a great level of detail. I do have some observations that can help with the context of the component and the requirements that we're trying to achieve:
> The fix that has makes the most sense to me, with my current knowledge and understanding of what's happening, is to utilize the propertyComparator function right below defaultSortComparator as the default sort function used and add an optional property called sortByProperty to Select that's default value is 'label' that I call and assign to sortOperator property on Select like so sortComparator = propertyComparator(sortByProperty). This will have the default sorting be done by label and allow the option to pass in a string property value to sort by a specific property.
The important point here is to preserve the `sortComparator` function as a parameter to the component. This flexibility is important because we can have unique sorting requirements. One example would be the dates where the value contains a specific string that should be interpreted to provide the correct sorting order. Another example would be using multiple properties to calculate the sorting. The `defaultSortComparator` is meant to be simple and handle the most common cases. The `propertyComparator` is a helper function that provides one common pattern of sorting that compares a single property using the equal sign. We can have other flavors of helpers for dates, multiple properties, etc. We should probably move the `propertyComparator` outside of the `Select` component.
> From what I could see main purpose of most of the sorting in the component was using it as a way to have the options go back to their original order once an option is deselected instead of having it sit at the top of the options. That makes sense but since the options were in a order that isn't easily replicated by comparing any of its values, I feel it made the most sense to create a dictionary or hash map of that original order and sort the values by that.
This solution was discussed during development but we have a requirement that invalidated this approach. The Select component can be asynchronous, paginated, and with search capabilities. That means that the order that the results are being fetched is not necessarily the correct order of the data. Imagine that we have a Select with the names of people. When we open the Select the first page loads the people with names that start with the letter A. If we scroll down, the next page is loaded, let's say with names that start with the letter B. So far the order is the same, but now the user searches for W and later for L. The order that we get the results is different from the order of the data. As we also don't know what's the sorting algorithm, we can't sort this dictionary. That's why we need the `sortComparator`, each place when the Select is applied knows what's the correct sorting algorithm and can configure it appropriately.
> Dates are sent in sorted from smallest time to largest time and are then sorted by label and displayed unsorted. I am of the opinion that dates should go from smallest to largest but with these, we can't sort by either label or value because neither would work or make sense. This is what the label and value look like for some {label: "Hour", value: "PT1H"}, {label: "Minute", value: "PT1M"}.
> The problem is that the numbers that are being passed in with a string label and a number value ex: { label: '20', value: 20 }. When the Select is inside of a SelectControl (superset-frontend/src/explore/components/controls/SelectControl.jsx) like it is when it is being used for viz plugins, the numbers are always being passed in with a label thanks to formatting.
I think for these examples, a possible solution would be to create another set of comparator helpers that deal with these specific scenarios and pass them when needed. Similar to `propertyComparator`.
> No matter what ends up being the solution to this, I believe it is a must to add more tests to all of the current tests it has to verify that it works with numbers as expected.
You're totally right here! Tests are essential since this component is widely used throughout the application and every change is of high risk.
I'm currently on PTO but I'll try to keep an eye on this PR and help any way I can. Thanks again for bringing up these points of improvement and writing such an amazing description.
--
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 edited a comment on pull request #17638: fix(select): [WIP] select component sort functionality on certain options
Posted by GitBox <gi...@apache.org>.
corbinrobb edited a comment on pull request #17638:
URL: https://github.com/apache/superset/pull/17638#issuecomment-988404019
## Update 2
Thank you everyone for all of the comments and help while looking at this with me! Also thank you Michael for the very detailed explanation, it really helped me out.
Alrighty, I reverted everything that I did within the Select component because there was a simpler way that I did not notice. Sorry about this ridiculous PR. I thought the solution would be simple but the component is widely used and versatile so it took me a whole lot of searching, reading, and manual testing.
### Problem
Mostly the same. There are Select components being made that are being passed options from SelectControl and they are being constructed from arrays of mostly static and mostly hardcoded data. From all of the ones that I have seen the majority seem to expect that data to retain its order.
I have come to this conclusion after looking at the nearly 200 SelectControl configuration objects/functions that are being used to make a lot Select components in the explore dashboard. I began writing a list of them to share where they are and where the data is created or exists, but it was incredibly tedious and I gave up after about 80.
I will just tell you how to find them. All you need to do is do a project-wide search for SearchControl, and then exclude the translation files or just scroll past them. You should get a little more than 200 hits where you will find objects that look something like this
```javascript
{
name: 'subdomain_granularity',
config: {
type: 'SelectControl',
label: t('Subdomain'),
default: 'day',
choices: formatSelectOptions([
'min',
'hour',
'day',
'week',
'month',
]),
description: t(
'The time unit for each block. Should be a smaller unit than ' +
'domain_granularity. Should be larger or equal to Time Grain',
),
},
},
```
The data that will make up the Select options are in `choices`, `options`, or in a `mapStateToProps`. A lot of them get formatted with a function like the one above called `formatSelectOptions` This creates an array that is ready to be passed to Select so it can make some options. Normally makes them look like this `[value, String(value)]`.
Most of the data for these are hard-coded values like in the example above. While the data is not all unique, there still are separate arrays being made for most and this is especially true for legacy plugins. Some of the data is unsorted datasource columns and those are the ones I felt needed `sortComparators` added to them. Everything else appears in order or at least in the intended order.
If you want to confirm how all of this looks you can do the search for SelectControls and you will be able to see for yourself.
There are Select components not being made within SelectControl like popovers, modals, and other things. From what I could see all of those had been accounted for and are displayed in an appropriate order.
### New FIx
Okay. When the fix for the top options going back to their correct positions after deselection was added, I saw that some objects were added an `order` key with its array index as the value. If I am not mistaken, this was done to preserve the order for these data arrays because there are values like operators, dates, and other such values.
[example of data with order keys](https://github.com/apache/superset/blob/9121e4555dd74915319de714536b053eb8d1f176/superset-frontend/src/explore/components/controls/ConditionalFormattingControl/FormattingPopoverContent.tsx#L46-L59)
These order indexes function the same way as the initial options index hashmap that I was using in a previous attempt and I'm all for using them for the data that gets passed into SelectControls when the plugins are made.
In SelectControl we have a method that iterates the formatted data array and returns objects for the Select component to build options.
This is the place where the data I am concerned with pass through. `c` is an item in a `choices` array
```javascript
if (Array.isArray(c)) {
const [value, label] = c.length > 1 ? c : [c[0], c[0]];
return {
value,
label,
};
}
```
Add `i` for index on the map function, check if there is not a `sortComparator`(more on that in a moment) and if there is not then add an order key to sort in order.
Since most of the chart/graph plugins are sending data that is either sorted or being passed in its intended order, this fixes the out-of-order data for the select dropdowns that are being made with SelectControl.
Now we can just add sortComparator where they make sense, like the columns from a datasource. And those will be sorted as we would like.
I chose not to import the `propertyComparator` for these and just used anon arrow functions. I did this because of the distance from the component and because the other imports in those files were from short distances.
I do not have solid reasoning for doing it this way, it was just a gut feeling that I should. If anybody disagrees with that I am happy to change it.
The last thing is setting a conditional in SelectControl that checks for a `sortComparator` value and if one doesn't exist then sort by order using `propertyComparator`
```javascript
sortComparator: this.props.sortComparator || propertyComparator('order')
```
That's the fix! There is not a whole lot going on and there are other ways but I feel like this is quick and simple.
I did get the implementation that I was making earlier working and got it up and going it working with the pagination and everything else but I abandoned it last minute for this fix because it is simpler and requires fewer changes.
### Other little fixes done on my journey
### legacy heatmap chart
There was a bug with the legacy heatmap chart within two of the Select components named `xscale_interval` and `yscale_interval`. This was from the config passing a default value of `"1"` and the choices data object passed uses numbers and start at `1`.
The Select component doesn't know where to put the one and will duplicate it after clicking other options. Setting default to the number one instead of the string fixes this.
**Before**
<img width="421" alt="Screen Shot 2021-12-07 at 4 03 48 PM" src="https://user-images.githubusercontent.com/31329271/145119469-b550e1e1-aee8-4226-9132-de963ff2e35c.png">
**After**
<img width="421" alt="Screen Shot 2021-12-07 at 4 08 03 PM" src="https://user-images.githubusercontent.com/31329271/145119899-bfec4e86-da81-4ec5-85aa-e4c339ac4695.png">
### AdhocFilterEditPopoverSimpleTabContent
The operators Select was not passed the order key or the sortComparator and the options appeared out of order. Then the Select holding the column values were being sorted by the label which worked when the values were string but sorted them out of order on numbers.
I added an order key and comparator for the operators Select. Then for the column values Select I added a condition that will sort by the values when they are numbers and by label if not.
**Before**
<img width="421" alt="Screen Shot 2021-12-07 at 4 40 02 PM" src="https://user-images.githubusercontent.com/31329271/145122740-872b755b-3015-4c8f-a0d7-3bd6cea76984.png">
<img width="421" alt="Screen Shot 2021-12-07 at 4 35 37 PM" src="https://user-images.githubusercontent.com/31329271/145122417-87b40352-3747-4796-a7df-5e05a91ab315.png">
**After**
<img width="421" alt="Screen Shot 2021-12-07 at 4 41 45 PM" src="https://user-images.githubusercontent.com/31329271/145122901-d07526b5-9f81-41d9-b8e5-dd1523ad91c3.png">
<img width="421" alt="Screen Shot 2021-12-07 at 4 42 19 PM" src="https://user-images.githubusercontent.com/31329271/145122957-a9de03af-955a-431b-844a-b224107d7231.png">
### Select sorting initial state
Caught this while writing this and I meant to have it in the last commit. I will push right after this comment.
The initial options being passed directly into the state works fine for most cases but when you can select multiple options it will do a jump while the dropdown is open and it throws all the options back into what the state's initial options were. I am not sure what is causing the jump and I couldn't get it to stop.
Anyways, having it jump from ordered to unordered is confusing so I just have the options sorted when they are initialized in the `selectOptions` useState hook.
```javascript
const [selectOptions, setSelectOptions] = useState<OptionsType>(
initialOptions.sort(sortComparator),
);
```
Still jumps from the selected options being at the top to having them back in order.
It isn't great but it is better than having it jump out of order.
**Before**
**After**
--
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 #17638: fix(select): [WIP] select component sort functionality on certain options
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #17638:
URL: https://github.com/apache/superset/pull/17638#issuecomment-988305524
# [Codecov](https://codecov.io/gh/apache/superset/pull/17638?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 [#17638](https://codecov.io/gh/apache/superset/pull/17638?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (87a8432) into [master](https://codecov.io/gh/apache/superset/commit/b5d13d72f209132b6a334bf1978e96cbc06026d9?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (b5d13d7) will **increase** coverage by `0.21%`.
> The diff coverage is `44.44%`.
> :exclamation: Current head 87a8432 differs from pull request most recent head 4782473. Consider uploading reports for the commit 4782473 to get more accurate results
[](https://codecov.io/gh/apache/superset/pull/17638?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 #17638 +/- ##
==========================================
+ Coverage 68.55% 68.77% +0.21%
==========================================
Files 1602 1597 -5
Lines 65354 65214 -140
Branches 6994 6953 -41
==========================================
+ Hits 44806 44848 +42
+ Misses 18666 18479 -187
- Partials 1882 1887 +5
```
| Flag | Coverage Δ | |
|---|---|---|
| javascript | `57.45% <44.44%> (+0.34%)` | :arrow_up: |
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/17638?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...et-ui-chart-controls/src/shared-controls/index.tsx](https://codecov.io/gh/apache/superset/pull/17638/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-c3VwZXJzZXQtZnJvbnRlbmQvcGFja2FnZXMvc3VwZXJzZXQtdWktY2hhcnQtY29udHJvbHMvc3JjL3NoYXJlZC1jb250cm9scy9pbmRleC50c3g=) | `38.53% <0.00%> (-0.36%)` | :arrow_down: |
| [...ns/legacy-plugin-chart-heatmap/src/controlPanel.ts](https://codecov.io/gh/apache/superset/pull/17638/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-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9sZWdhY3ktcGx1Z2luLWNoYXJ0LWhlYXRtYXAvc3JjL2NvbnRyb2xQYW5lbC50cw==) | `80.00% <ø> (ø)` | |
| [...nd/plugins/plugin-chart-table/src/controlPanel.tsx](https://codecov.io/gh/apache/superset/pull/17638/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-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9wbHVnaW4tY2hhcnQtdGFibGUvc3JjL2NvbnRyb2xQYW5lbC50c3g=) | `21.34% <0.00%> (-0.50%)` | :arrow_down: |
| [superset-frontend/src/components/Select/Select.tsx](https://codecov.io/gh/apache/superset/pull/17638/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvU2VsZWN0L1NlbGVjdC50c3g=) | `86.33% <ø> (ø)` | |
| [...l/AdhocFilterEditPopoverSimpleTabContent/index.tsx](https://codecov.io/gh/apache/superset/pull/17638/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9jb250cm9scy9GaWx0ZXJDb250cm9sL0FkaG9jRmlsdGVyRWRpdFBvcG92ZXJTaW1wbGVUYWJDb250ZW50L2luZGV4LnRzeA==) | `67.15% <66.66%> (-0.26%)` | :arrow_down: |
| [.../src/explore/components/controls/SelectControl.jsx](https://codecov.io/gh/apache/superset/pull/17638/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9jb250cm9scy9TZWxlY3RDb250cm9sLmpzeA==) | `56.89% <66.66%> (-2.04%)` | :arrow_down: |
| [...packages/superset-ui-core/src/query/types/Query.ts](https://codecov.io/gh/apache/superset/pull/17638/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-c3VwZXJzZXQtZnJvbnRlbmQvcGFja2FnZXMvc3VwZXJzZXQtdWktY29yZS9zcmMvcXVlcnkvdHlwZXMvUXVlcnkudHM=) | `100.00% <0.00%> (ø)` | |
| [...ugins/legacy-plugin-chart-calendar/src/Calendar.js](https://codecov.io/gh/apache/superset/pull/17638/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-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9sZWdhY3ktcGx1Z2luLWNoYXJ0LWNhbGVuZGFyL3NyYy9DYWxlbmRhci5qcw==) | `0.00% <0.00%> (ø)` | |
| [...es/superset-ui-core/src/query/buildQueryContext.ts](https://codecov.io/gh/apache/superset/pull/17638/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-c3VwZXJzZXQtZnJvbnRlbmQvcGFja2FnZXMvc3VwZXJzZXQtdWktY29yZS9zcmMvcXVlcnkvYnVpbGRRdWVyeUNvbnRleHQudHM=) | `100.00% <0.00%> (ø)` | |
| ... and [18 more](https://codecov.io/gh/apache/superset/pull/17638/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/17638?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/17638?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 [b5d13d7...4782473](https://codecov.io/gh/apache/superset/pull/17638?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] corbinrobb commented on pull request #17638: fix(select): [WIP] select component sort functionality on certain options
Posted by GitBox <gi...@apache.org>.
corbinrobb commented on pull request #17638:
URL: https://github.com/apache/superset/pull/17638#issuecomment-988404019
### Another Update
Thank you everyone for all of the comments and help while looking at this with me! Also thank you Michael for the very detailed explanation, it really helped me out.
Alrighty, I reverted everything that I did within the Select component because there was a simpler way that I did not notice. Sorry about this ridiculous PR. I thought the solution would be simple but the component is widely used and versatile so it took me a whole lot of searching, reading, and manual testing.
### Problem
Mostly the same. There are Select components being made that are being passed options from SelectControl and they are being constructed from arrays of mostly static and mostly hardcoded data. From all of the ones that I have seen the majority seem to expect that data to retain its order.
There are Select components not being made within SelectControl like popovers, modals, and other things. From what I could see all of those had been accounted for and are displayed in an appropriate order.
I have come to this conclusion after looking at the nearly 200 SelectControl configuration objects/functions that are being used to make a lot Select components in the explore dashboard. I began writing a list of them to share where they are and where the data is created or exists, but it was incredibly tedious and I gave up after about 80.
I will just tell you how to find them. All you need to do is do a project-wide search for SearchControl, and then exclude the translation files or just scroll past them. You should get a little more than 200 hits where you will find objects that look something like this
```javascript
{
name: 'subdomain_granularity',
config: {
type: 'SelectControl',
label: t('Subdomain'),
default: 'day',
choices: formatSelectOptions([
'min',
'hour',
'day',
'week',
'month',
]),
description: t(
'The time unit for each block. Should be a smaller unit than ' +
'domain_granularity. Should be larger or equal to Time Grain',
),
},
},
```
The data that will make up the Select options are in `choices`, `options`, or in a `mapStateToProps`. A lot of them get formatted with a function like the one above called `formatSelectOptions` This creates an array that is ready to be passed to Select so it can make some options. Normally makes them look like this `[value, String(value)]`.
Most of the data for these are hard-coded values like in the example above. While the data is not all unique, there still are separate arrays being made for most and this is especially true for legacy plugins. Some of the data is unsorted datasource columns and those are the ones I felt needed `sortComparators` added to them. Everything else appears in order or at least in the intended order.
If you want to confirm how all of this looks you can do the search for SelectControls and you will be able to see for yourself.
### New FIx
Okay. When the fix for the top options going back to their correct positions after deselection was added, I saw that some objects were added an `order` key with its array index as the value. If I am not mistaken, this was done to preserve the order for these data arrays because there are values like operators, dates, and other such values.
[example of data with order keys](https://github.com/apache/superset/blob/9121e4555dd74915319de714536b053eb8d1f176/superset-frontend/src/explore/components/controls/ConditionalFormattingControl/FormattingPopoverContent.tsx#L46-L59)
These order indexes function the same way as the initial options index hashmap that I was using in a previous attempt and I'm all for using them for the data that gets passed into SelectControls when the plugins are made.
In SelectControl we have a method that iterates the formatted data array and returns objects for the Select component to build options.
This is the place where the data I am concerned with pass through. `c` is an item in a `choices` array
```javascript
if (Array.isArray(c)) {
const [value, label] = c.length > 1 ? c : [c[0], c[0]];
return {
value,
label,
};
}
```
Add `i` for index on the map function, check if there is not a `sortComparator`(more on that in a moment) and if there is not then add an order key to sort in order.
Since most of the chart/graph plugins are sending data that is either sorted or being passed in its intended order, this fixes the out-of-order data for the select dropdowns that are being made with SelectControl.
Now we can just add sortComparator where they make sense, like the columns from a datasource. And those will be sorted as we would like.
I chose not to import the `propertyComparator` for these and just used anon arrow functions. I did this because of the distance from the component and because the other imports in those files were from short distances.
I do not have solid reasoning for doing it this way, it was just a gut feeling that I should. If anybody disagrees with that I am happy to change it.
The last thing is setting a conditional in SelectControl that checks for a `sortComparator` value and if one doesn't exist then sort by order using `propertyComparator`
```javascript
sortComparator: this.props.sortComparator || propertyComparator('order')
```
That's the fix! There is not a whole lot going on and there are other ways but I feel like this is quick and simple.
I did get the implementation that I was making earlier working and got it up and going it working with the pagination and everything else but I abandoned it last minute for this fix because it is simpler and requires fewer changes.
### Other little fixes done on my journey
### legacy heatmap chart
There was a bug with the legacy heatmap chart within two of the Select components named `xscale_interval` and `yscale_interval`. This was from the config passing a default value of `"1"` and the choices data object passed uses numbers and start at `1`.
The Select component doesn't know where to put the one and will duplicate it after clicking other options. Setting default to the number one instead of the string fixes this.
**Before**
<img width="421" alt="Screen Shot 2021-12-07 at 4 03 48 PM" src="https://user-images.githubusercontent.com/31329271/145119469-b550e1e1-aee8-4226-9132-de963ff2e35c.png">
**After**
<img width="421" alt="Screen Shot 2021-12-07 at 4 08 03 PM" src="https://user-images.githubusercontent.com/31329271/145119899-bfec4e86-da81-4ec5-85aa-e4c339ac4695.png">
### AdhocFilterEditPopoverSimpleTabContent
The operators Select was not passed the order key or the sortComparator and the options appeared out of order. Then the Select holding the column values were being sorted by the label which worked when the values were string but sorted them out of order on numbers.
I added an order key and comparator for the operators Select. Then for the column values Select I added a condition that will sort by the values when they are numbers and by label if not.
**Before**
<img width="421" alt="Screen Shot 2021-12-07 at 4 40 02 PM" src="https://user-images.githubusercontent.com/31329271/145122740-872b755b-3015-4c8f-a0d7-3bd6cea76984.png">
<img width="421" alt="Screen Shot 2021-12-07 at 4 35 37 PM" src="https://user-images.githubusercontent.com/31329271/145122417-87b40352-3747-4796-a7df-5e05a91ab315.png">
**After**
<img width="421" alt="Screen Shot 2021-12-07 at 4 41 45 PM" src="https://user-images.githubusercontent.com/31329271/145122901-d07526b5-9f81-41d9-b8e5-dd1523ad91c3.png">
<img width="421" alt="Screen Shot 2021-12-07 at 4 42 19 PM" src="https://user-images.githubusercontent.com/31329271/145122957-a9de03af-955a-431b-844a-b224107d7231.png">
### Select sorting initial state
Caught this while writing this and I meant to have it in the last commit. I will push right after this comment.
The initial options being passed directly into the state works fine for most cases but when you can select multiple options it will do a jump while the dropdown is open and it throws all the options back into what the state's initial options were. I am not sure what is causing the jump and I couldn't get it to stop.
Anyways, having it jump from ordered to unordered is confusing so I just have the options sorted when they are initialized in the `selectOptions` useState hook.
```javascript
const [selectOptions, setSelectOptions] = useState<OptionsType>(
initialOptions.sort(sortComparator),
);
```
Still jumps from the selected options being at the top to having them back in order.
It isn't great but it is better than having it jump out of order.
**Before**
**After**
--
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 #17638: fix(select): select component sort functionality on certain options
Posted by GitBox <gi...@apache.org>.
AAfghahi commented on pull request #17638:
URL: https://github.com/apache/superset/pull/17638#issuecomment-989116006
/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 edited a comment on pull request #17638: fix(select): [WIP] select component sort functionality on certain options
Posted by GitBox <gi...@apache.org>.
corbinrobb edited a comment on pull request #17638:
URL: https://github.com/apache/superset/pull/17638#issuecomment-988404019
### Another Update
Thank you everyone for all of the comments and help while looking at this with me! Also thank you Michael for the very detailed explanation, it really helped me out.
Alrighty, I reverted everything that I did within the Select component because there was a simpler way that I did not notice. Sorry about this ridiculous PR. I thought the solution would be simple but the component is widely used and versatile so it took me a whole lot of searching, reading, and manual testing.
### Problem
Mostly the same. There are Select components being made that are being passed options from SelectControl and they are being constructed from arrays of mostly static and mostly hardcoded data. From all of the ones that I have seen the majority seem to expect that data to retain its order.
I have come to this conclusion after looking at the nearly 200 SelectControl configuration objects/functions that are being used to make a lot Select components in the explore dashboard. I began writing a list of them to share where they are and where the data is created or exists, but it was incredibly tedious and I gave up after about 80.
I will just tell you how to find them. All you need to do is do a project-wide search for SearchControl, and then exclude the translation files or just scroll past them. You should get a little more than 200 hits where you will find objects that look something like this
```javascript
{
name: 'subdomain_granularity',
config: {
type: 'SelectControl',
label: t('Subdomain'),
default: 'day',
choices: formatSelectOptions([
'min',
'hour',
'day',
'week',
'month',
]),
description: t(
'The time unit for each block. Should be a smaller unit than ' +
'domain_granularity. Should be larger or equal to Time Grain',
),
},
},
```
The data that will make up the Select options are in `choices`, `options`, or in a `mapStateToProps`. A lot of them get formatted with a function like the one above called `formatSelectOptions` This creates an array that is ready to be passed to Select so it can make some options. Normally makes them look like this `[value, String(value)]`.
Most of the data for these are hard-coded values like in the example above. While the data is not all unique, there still are separate arrays being made for most and this is especially true for legacy plugins. Some of the data is unsorted datasource columns and those are the ones I felt needed `sortComparators` added to them. Everything else appears in order or at least in the intended order.
If you want to confirm how all of this looks you can do the search for SelectControls and you will be able to see for yourself.
There are Select components not being made within SelectControl like popovers, modals, and other things. From what I could see all of those had been accounted for and are displayed in an appropriate order.
### New FIx
Okay. When the fix for the top options going back to their correct positions after deselection was added, I saw that some objects were added an `order` key with its array index as the value. If I am not mistaken, this was done to preserve the order for these data arrays because there are values like operators, dates, and other such values.
[example of data with order keys](https://github.com/apache/superset/blob/9121e4555dd74915319de714536b053eb8d1f176/superset-frontend/src/explore/components/controls/ConditionalFormattingControl/FormattingPopoverContent.tsx#L46-L59)
These order indexes function the same way as the initial options index hashmap that I was using in a previous attempt and I'm all for using them for the data that gets passed into SelectControls when the plugins are made.
In SelectControl we have a method that iterates the formatted data array and returns objects for the Select component to build options.
This is the place where the data I am concerned with pass through. `c` is an item in a `choices` array
```javascript
if (Array.isArray(c)) {
const [value, label] = c.length > 1 ? c : [c[0], c[0]];
return {
value,
label,
};
}
```
Add `i` for index on the map function, check if there is not a `sortComparator`(more on that in a moment) and if there is not then add an order key to sort in order.
Since most of the chart/graph plugins are sending data that is either sorted or being passed in its intended order, this fixes the out-of-order data for the select dropdowns that are being made with SelectControl.
Now we can just add sortComparator where they make sense, like the columns from a datasource. And those will be sorted as we would like.
I chose not to import the `propertyComparator` for these and just used anon arrow functions. I did this because of the distance from the component and because the other imports in those files were from short distances.
I do not have solid reasoning for doing it this way, it was just a gut feeling that I should. If anybody disagrees with that I am happy to change it.
The last thing is setting a conditional in SelectControl that checks for a `sortComparator` value and if one doesn't exist then sort by order using `propertyComparator`
```javascript
sortComparator: this.props.sortComparator || propertyComparator('order')
```
That's the fix! There is not a whole lot going on and there are other ways but I feel like this is quick and simple.
I did get the implementation that I was making earlier working and got it up and going it working with the pagination and everything else but I abandoned it last minute for this fix because it is simpler and requires fewer changes.
### Other little fixes done on my journey
### legacy heatmap chart
There was a bug with the legacy heatmap chart within two of the Select components named `xscale_interval` and `yscale_interval`. This was from the config passing a default value of `"1"` and the choices data object passed uses numbers and start at `1`.
The Select component doesn't know where to put the one and will duplicate it after clicking other options. Setting default to the number one instead of the string fixes this.
**Before**
<img width="421" alt="Screen Shot 2021-12-07 at 4 03 48 PM" src="https://user-images.githubusercontent.com/31329271/145119469-b550e1e1-aee8-4226-9132-de963ff2e35c.png">
**After**
<img width="421" alt="Screen Shot 2021-12-07 at 4 08 03 PM" src="https://user-images.githubusercontent.com/31329271/145119899-bfec4e86-da81-4ec5-85aa-e4c339ac4695.png">
### AdhocFilterEditPopoverSimpleTabContent
The operators Select was not passed the order key or the sortComparator and the options appeared out of order. Then the Select holding the column values were being sorted by the label which worked when the values were string but sorted them out of order on numbers.
I added an order key and comparator for the operators Select. Then for the column values Select I added a condition that will sort by the values when they are numbers and by label if not.
**Before**
<img width="421" alt="Screen Shot 2021-12-07 at 4 40 02 PM" src="https://user-images.githubusercontent.com/31329271/145122740-872b755b-3015-4c8f-a0d7-3bd6cea76984.png">
<img width="421" alt="Screen Shot 2021-12-07 at 4 35 37 PM" src="https://user-images.githubusercontent.com/31329271/145122417-87b40352-3747-4796-a7df-5e05a91ab315.png">
**After**
<img width="421" alt="Screen Shot 2021-12-07 at 4 41 45 PM" src="https://user-images.githubusercontent.com/31329271/145122901-d07526b5-9f81-41d9-b8e5-dd1523ad91c3.png">
<img width="421" alt="Screen Shot 2021-12-07 at 4 42 19 PM" src="https://user-images.githubusercontent.com/31329271/145122957-a9de03af-955a-431b-844a-b224107d7231.png">
### Select sorting initial state
Caught this while writing this and I meant to have it in the last commit. I will push right after this comment.
The initial options being passed directly into the state works fine for most cases but when you can select multiple options it will do a jump while the dropdown is open and it throws all the options back into what the state's initial options were. I am not sure what is causing the jump and I couldn't get it to stop.
Anyways, having it jump from ordered to unordered is confusing so I just have the options sorted when they are initialized in the `selectOptions` useState hook.
```javascript
const [selectOptions, setSelectOptions] = useState<OptionsType>(
initialOptions.sort(sortComparator),
);
```
Still jumps from the selected options being at the top to having them back in order.
It isn't great but it is better than having it jump out of order.
**Before**
**After**
--
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 commented on a change in pull request #17638: fix(select): [WIP] select component sort functionality on certain options
Posted by GitBox <gi...@apache.org>.
eschutho commented on a change in pull request #17638:
URL: https://github.com/apache/superset/pull/17638#discussion_r762358677
##########
File path: superset-frontend/src/components/Select/Select.tsx
##########
@@ -289,17 +303,20 @@ const Select = ({
pageSize = DEFAULT_PAGE_SIZE,
placeholder = t('Select ...'),
showSearch = true,
- sortComparator = defaultSortComparator,
+ sortByProperty = 'label',
+ sortComparator = propertyComparator(sortByProperty),
+ sortOptions = false,
value,
...props
}: SelectProps) => {
const isAsync = typeof options === 'function';
const isSingleMode = mode === 'single';
const shouldShowSearch = isAsync || allowNewOptions ? true : showSearch;
const initialOptions =
- options && Array.isArray(options) ? options : EMPTY_OPTIONS;
- const [selectOptions, setSelectOptions] =
- useState<OptionsType>(initialOptions);
+ options && Array.isArray(options) ? [...options] : EMPTY_OPTIONS;
+ const [selectOptions, setSelectOptions] = useState<OptionsType>(
+ sortOptions ? initialOptions.sort(sortComparator) : initialOptions,
Review comment:
@corbinrobb it looks like if someone passes in a sortComparator, then their intention is to have the list sorted by that value, is that right? If so, can we make the presence of that prop the deciding factor on whether to sort or not? Something like:
```
const [selectOptions, setSelectOptions] = useState<OptionsType>(
!!sortComparator?.length ? initialOptions.sort(sortComparator) : initialOptions,
)
```
--
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