fix(editors): use indexOf in multiple select editor to load value #50
Conversation
indexOf must be used because defaultValue is an array. the current state is broken and values are not loaded when the multiple select list is opened
|
Hmm it does revert my previous commit, have you tested the sort descending which was a fix in that previous commit? I'm a bit surprised since it's a complete revert of my previous commit, which was a fix. Anyhow, I don't use this inline editor in any of my projects yet, so you would more than I do on this subject. For the string sorting, I would actually rather always sort by string as default. We might need another option to have another way to sort. If another option is created for that, we could take advantages of the built-in Sorters, these are used to sort data of local grid (without a Backend Service). |
|
Would I be testing the data sorted on the grid or in the select list? |
|
The previous commit I made was to fix the select list not being sorted correctly on a sort order descending. Does that answer your question? |
|
Yes, thank you. I will double check the sorting DESC. Not sure how much work I will be able to do over the next week (i will be "off work", but may still be working lol), but this will be the next thing I will look into. |
|
@jmzagorski
|
|
I reviewed example 3 again for sorting editors and added examples in 12 and 4 for filters. If you pull down the branch I can't find any problems with sorting. Could I be missing something you saw? The Also, the Another thing to be aware of, if there is a DESC sortBy set on a filter, the blank default option will be thrown to the bottom. You can see this in my example 12 I added. |
|
Oh ok your code change is only for the For the Not sure to understand your last comment, when you say filter do you mean on |
|
My last comment was about example 12. If you run this pull request's branch and look at the last column on example 12, i sorted it descending and the default selection with a value of '' is at the bottom |
|
oh yeah I guess that is expected for a sort DESC lol. Not much we can do about that I think. Unless we fix it with some kind of patch but that is kind of ugly |
this was not used, but we should check for columnDef.filter or columnDef.params, whichever is being used
|
I removed the |
|
Oh yeah that does look good. Where you done with the PR then, anything else to review before the merge? I'm good with your changes but I didn't test locally and I don't think I need to since you tested yourself. |
|
i am playing around with all the examples to see if an exceptions are thrown and if anything weird happens. After that I can merge it if you want. Not related, do you notice the inline date editors not working? I select a date and it does not set it. Specifically example 3. |
|
Yup go ahead and merge whenever ready 👍 There's also this other PR that you can merge after reviewing. That one, I was going to merge it myself but I know you use Inline Editor, so I left it open for you to be aware of, there's some description in it |
|
Oh yea I saw that PR and added a review I never submitted last night. I submitted it now |
was added to show example with sorting/filtering before service logic was changed to now check gridOptions.params
indexOfmust be used becausedefaultValueis an array for themultipleSelectEditor. the currentmultipleSelectEditoris broken and values are not loaded when the multiple select list is opened.This reverts part of this commit for the
multipleSelectEditor.The excel export and the sort still works with numbers. However, the current state of sorters will not sort "Task 2" and "Task 10" correctly regardless of the editors because those values are strings and not numbers. A separate issue would have to be opened to that. I updated
Example3.tsto show that problem by allowing in Tasks greater than 10 in the prerequisite column.If I can figure out/have permission to release a hot fix (version 1.12.3) I will do that because I would imagine this is critical since it breaks inline editing.