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