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