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

Fri, 03 Dec 2021 08:56:58 -0800 


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";>
   
   ![Screen Recording 2021-12-03 at 9 44 54 
AM](https://user-images.githubusercontent.com/31329271/144640594-d589727c-9008-4235-bd95-08b96d3b434c.gif)
   
   ### 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";>
   
   ![Screen Recording 2021-12-03 at 9 46 05 
AM](https://user-images.githubusercontent.com/31329271/144640771-30049135-f702-4aeb-8efb-ec0d14336754.gif)
   
   


-- 
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