Open dacook opened 1 month ago
I think the consequence can be this behavior (easier to reproduce as super admin), if the user clicks several dropdowns within a short time: https://www.loom.com/share/94708479cbb74e9c87428294459d7760?sid=a6e3b4b0-d1d1-44f8-b5ef-8d7e75535902
Hello @dacook , @filipefurtad0 ,
May I work on this issue ?
Hi @cyrillefr yes feel free to pick, I will assign you: but it might be a tough one! Don't hesitate to ping David or Filipe again if you get stuck :)
Hello @dacook ,
From what I have read from the source code, data that populate the selects come from the Rails controller.
It is populated for each row in app/views/admin products_v3/_product_row.html.haml
via
the SearchableDropdownComponent
component (that uses tom-select).
The load
once function you mention from the tom-select
lib is for remote data fetching.
So I guess your idea would be to on an event(click) to populate the select with the load once. It could avoid populating every lists, the majority of them the user would not work on.
But since, one cannot use the load once here, there would be the possibility to use some other method of the api, like the addItem
, and populate the select(on a click event), while storing the data in a JS Array somewhere.
At the time I write this, I do not know if I can use hotwired/stimulus, I need to check :)
Also, when I talk about load once not possible, it is because I did not see some fetch/ajax in the source code and since you want to depart from Angular, my guess is that you would not want that. (I might be wrong though).
NB you mentioned the supplier lists, should the switch be applied to every tom-select list ?
That would imply a new component that respond to click event or to pass an option to the existing one (if possible).
~~Hi @cyrillefr - Really sorry for getting back on this late.
I believe creating a new component for the tom-select specifically for fetching options from our server would be better. This could be more manageable I guess. You may refer to the below, I think we would want something like this: https://tom-select.js.org/examples/remote/
The load once function you mention from the tom-select lib is for remote data fetching.
Yes, exactly but it caches the searched results as well. Not exactly caching, it just appends the options in the DOM and then looks at them before making the API call. I think it's an optimized problem like this.
NB you mentioned the supplier lists, should the switch be applied to every tom-select list ?
I think, applying this to every tom-select might be an overkill for now. However, a new component would allow us to reuse that specific tom-select for any place where we might face a similar issue.
@dacook - What do you think about it?~~
Really sorry @cyrillefr I missed the use case above 😕 That way we won't be showing options on the UI, we would have to search for it
I guess then we need to use stimulus here. We could create a generic stimulus controller which would fetch all the options from the server by the specified path and then add those options in the specified tom-select's options dynamically.
Now we need to design this controller and see how we could maintain which path to fetch the options from and how we could instruct the controller to update the specific tom-select's options.
Behind the scenes, tom-select uses the html select element, so I believe we could manipulate its options by JS.
For now we need to make it for this select field option.
Please let me know if you have any questions. Thanks.
Hello @chahmedejaz ,
Thanks for the comments.
The important point in my opinion here would be to populate options upon some event (click), so that only the selects that are going to be used are populated. Except for the default I guess.
Otherwise, populating every selects the way it is done currently or populating them by JS would be the same( except browser would work a bit more and server a bit less). From what I understand, populating each and everyone select is the main issue here.
Do you see things that way too ?
The important point in my opinion here would be to populate options upon some event (click), so that only the selects that are going to be used are populated
@cyrillefr - Yes, this would indeed be a good solution in terms of having a clean DOM, however, this would cause some network delay on the UI before rendering the options. This might not be a good user experience to wait for the options upon clicking the dropdown. I've some rough idea about how we might do that without having much delay, however I think that might add an extra layer of complexity on the Stimulus side.
From what I understand, populating each and everyone select is the main issue here.
I believe the main issue here is that the server is responding with a lot of duplicated options in the html. This is not an optimised solution in terms of response size. By delegating the options responsibility to the JS would greatly reduce the html response size. Moreover, this may reduce the network latency of the products page response as well.
Makes sense?
Hi both, thanks for discussing this. I'm sorry, I think I forgot to respond at the start of the week.
The question is, what's the problem we're trying to solve? I have to admit I haven't measured any performance bottlenecks so I don't have an answer sorry. I raised it because I thought it was likely to cause problems, and it may have (see Filipe's post), but I haven't investigated yet. So I hadn't prioritised the task. So if you're willing, it would be good to try and replicate Filipe's issue and see how we can fix that.
But now I'm thinking about it...
I guess there's a few levels we can optimise at, and I'm not sure what's practical. I think of at at a few levels:
Feel free to investigate further and try out what you think would be best.
What we should change and why (this is tech debt)
Since adding dropdowns to the bulk edit screen (
/admin/products
with feature toggleadmin_style_v3
enabled), I've noticed the contents of the select lists are repeated for every row.It's not too bad for short lists. But a super-admin user has access to all suppliers on the system, which can be a very long list.
Proposed solution
Each type of dropdown shares the same list of options (apart from the
selected
attr), so we could probably load it once, then initialise the dropdowns with JS. I think Tomselect already has options for using a data source like that.