-
-
Notifications
You must be signed in to change notification settings - Fork 715
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[BUU] Producers selects are to be populated on focus #12524
base: master
Are you sure you want to change the base?
[BUU] Producers selects are to be populated on focus #12524
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice approach ! I just have one suggestion to make it a little bit better, see my comment.
Also did you try to reproduce the issue demonstrated by Filipe with this change: #12482 (comment) ?
Hello @rioug , I have not tested the case shown by @filipefurtad0. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very clever solution.
Can you elaborate a bit more what the advantage of this is?
Effects that I could imagine, but I haven't measured any of this:
- The transferred HTML is smaller. This doesn't really matter with compression though because repetitions are well compacted.
- The browser might render the page quicker because it doesn't have to render all the options to start with.
- The browser does more work on focus because it has to build the options which would otherwise already be rendered.
- Were the producer options cached before? If not then this new approach will save us a lot of database queries and building of options on the server. If it was cached then there's no big benefit in rendering it only once.
My main question is probably: Which solution is less work for the browser? And maybe the answer depends on the size of the product set and how many products you edit. If the list is big and you edit only one product, the new on-demand building of options is probably better. If the user edits every product on the page then it doesn't help.
I guess that you are on the right path here but I don't find the answer straight forward.
Anyway, very well done!
} | ||
|
||
fetchProducers() { | ||
if ((Object.keys(this.control.options).length) > 1) return false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if there is only one option? It will try to populate this on every focus, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, on each focus, fetchProducers will be called.
At first, there is only one option (the selected one), so the select will be populated.
After that, the number of options will always be superior to 1 and there will be not new attempt to populate the select.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, if the user only has access to one producer, it will always be 1. So we could add an additional check below, eg:
if(template.content.children.length <= 1) return; # by the way, is the false return value necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good know 👍 It will be interesting to see if resolved the issue that Filipe found.
%template{ id: "producer_options"} | ||
= options_for_select(producer_options, product.supplier_id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just came across this PR and had a quick look. Thanks for working on this!
This partial is repeated for every product row, yet it appears the intention was to only include the template once. When the controller searches for the element by ID, it will simply find the last one (standard browser behaviour).
This is ok, it would work, and as Maikel mentioned the compression will take care of most of it. But it looks like it should be very easy to remove the duplication by moving the template to another partial. So if you have time, feel free to go ahead update it ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aaah, my bad. I just pushed a correction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, but i can see:
- this is still repeated for every product.
producer_options
is no longer needed inproduct_row
options_for_select
doesn't need a second parameter for selected value (tomselect will need to choose it for each product when initialised)
But first let's see if we can confirm a performance improvement before progressing.
Hey @cyrillefr , I went through the scenarios you've suggested:
Unfortunately, I could still reproduce the issue: Indeed, it is related to fast pace clicking. It is noticeable how dropdowns take some time to open, after clicking. From a usability point of view, it is difficult to say, in a qualitative way, whether the PR brings improvement - although this was my first impression. In any case, I've tried to measure this with Lighthouse. Here are the results: Before this PR After this PR We can see that:
It's hard to say whether these results are meaningful. I guess we might need more runs to account for variability. In any case, and on this last point:
I'm not sure I understand the test case. I've switched to a producer with one product only, but can still find the search input in the page: What do you think @cyrillefr ? |
Hello @filipefurtad0 , I don't know how to improve the usability with long lists, maybe it has to do with too much data is select boxes. Regarding the input and the 10 threshold, it is the input box that appears on each select box that has 11 or more options and not the input that appears on top of the page. |
- added a new Stimulus controller to display one line selects - that are to be populated when getting focus to avoid repeating - 10 times same option lists - on focus, select is populated by the data stored in template. - data that used to be source for each select (and so repeated) is - now stored once in a template
- comments - id of template as a parameter - little clean-up
- added a new partial to avoid template repetition
bb4f9e4
to
9adbb1d
Compare
I just had a look at a super-admin user on current fr_prod and fr_staging, and found that there's already quite a delay when opening producer dropdowns. The reported increase in delay is a concern. It looks like there are different performance bottlenecks to look at. So from what I can see, unfortunately this PR doesn't seem to bring a benefit. But it has helped us learn a bit more about the performance, thanks for your contribution. I think we should pause on this, re-evaluate the problems and prioritise along with other priorities. I'll discuss in #backoffice-ui-uplift |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just had another look, and have some comments.
But unfortunately this doesn't change the problem that Filipe found: it was slower to respond when you first click on the dropdown.
I wonder if there's a faster way to load the producer options on demand. Maybe a single call to addOptions
is more efficient?
Or maybe instead of using the TomSelect API, can we manipulate the DOM and add the option div
s directly? We could pre-build the template in the TomSelect format like this (a random example copied):
<div data-selectable="" data-value="984" class="option" role="option" id="_products_0_variants_attributes_0_supplier_id-opt-811">Ingelara</div>
But I have no idea if that will actually work, or be any quicker..
Probalby what we need is a low-level TomSelect plugin that can re-use dom elements between selects. But I don't think that exists or would be easy to build.
Maybe we need something other than TomSelect. It would be interesting to compare performance with Select2 that is used on the old Bulk Edit Products screen. If it's faster could we just switch back to that one?
} | ||
|
||
fetchProducers() { | ||
if ((Object.keys(this.control.options).length) > 1) return false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, if the user only has access to one producer, it will always be 1. So we could add an additional check below, eg:
if(template.content.children.length <= 1) return; # by the way, is the false return value necessary?
const template = this.template(this.templateIdValue ); | ||
Array.from(template.content.children).forEach((option) => { | ||
this.control.addOption({value: option.value, text: option.text}); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that this would add the selected producer, which is already there, so it would appear twice. Before addOption
, I think we need to check if option.value
is already present in the list.
What? Why?
When visiting page
admin/products
(in V3 mode), each select is populated via the classical Rails way.But each select carries the same data, which can be some overload for the producers selects that may carry a lot of data.
My solution is instead to put the producers data once in a html element
template
.Each select is only populated with its default.
Then, on focus, it is populated.
Therefore, no more copying 10 times the same options for the 10 selects.
Also, it is now the browser job to populate the focused select.
One group of data is written on the page instead of 10, which reduces the server work.
I have created for that a Stimulus controller that inherit from tom-select, and not a StimulusReflex one.
What should we test?
admin/products
Release notes
Changelog Category (reviewers may add a label for the release notes):
The title of the pull request will be included in the release notes.
Dependencies
Documentation updates