[GitHub] [superset] michael-s-molina commented on pull request #17638: fix(select): [WIP] select component sort functionality on certain options

[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