[GitHub] [superset] corbinrobb opened a new pull request #17638: [WIP] fix(select): select component sort functionality on certain options

Thu, 02 Dec 2021 17:53:59 -0800 


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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]



---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Reply via email to