[16.0][ADD] web_sort_menu: Module web_sort_menu #3199
Conversation
a4867e9 to
aad2db6
Compare
|
|
| t-inherit-mode="extension" | ||
| owl="1" | ||
| > | ||
| <xpath expr="//header[@class='o_navbar']//t/t" position="replace"> |
There was a problem hiding this comment.
suggestion: Please use a more focused xpath: if you want to find the element that has t-set="apps", then you can use t[@t-set='apps'] in the xpath.
Searching for //t/t is difficult to maintain because it is not clear which node you are looking for.
There was a problem hiding this comment.
Thank you for the input. Updated as suggested.
| t-inherit-mode="extension" | ||
| owl="1" | ||
| > | ||
| <xpath expr="//header[@class='o_navbar']//t/t" position="replace"> |
There was a problem hiding this comment.
suggestion: replace is risky because other modules might expect that node to be present, so it should be avoided as much as possible.
In this case I suggest to go after the node that sets the apps variable and substitute its value with the sorted list.
web_sort_menu/__manifest__.py
Outdated
| "author": "Anusri Veerappan Prakasam, Odoo Community Association (OCA)", | ||
| "website": "https://github.com/OCA/web", | ||
| "license": "AGPL-3", | ||
| "depends": ["web", "web_responsive"], |
There was a problem hiding this comment.
suggestion
| "depends": ["web", "web_responsive"], | |
| "depends": ["web",], |
I can't find where the dependency from web_responsive is needed, could you please check if that's actually necessary?
Without web_responsive, I expect that the dropdown menu is sorted.
There was a problem hiding this comment.
I missed to think about dropdown menu getting sorted, I thought only NavBar would be sorted hence added web_responsive in the dependency.
I see dropdown menu getting sorted, hence removed web_responsive module as dependency.
|
|
||
| onWillStart(() => { | ||
| const apps = this.menuService.getApps(); | ||
| this.sortedApps = apps.sort((a, b) => a.name.localeCompare(b.name)); |
There was a problem hiding this comment.
note: It is worth noting that apps are sorted based on the translated name, so users with different languages will see them in a different order
There was a problem hiding this comment.
Yes, based on different languages sorting happens according to it.
Updated Description of module with this information.
There was a problem hiding this comment.
praise: Thanks for updating the documentation, this is something that developers rarely do in response to this kind of comments.
👍
| this.sortedApps = []; | ||
|
|
||
| onWillStart(() => { | ||
| const apps = this.menuService.getApps(); |
There was a problem hiding this comment.
thought: I don't think this search is that expensive, but we could avoid it by sorting directly in the template, or exposing a method and calling it in the template.
This would also preserve any edit that other modules have done to the apps variable in the template.
There was a problem hiding this comment.
I agree with you that we preseve any other module editing the value of apps,.
I tried to implement this search explicitly in the sort menu. However, I see we would need to use menuService in order to get information about available apps to sort them. If so, we could directly use the function exposed in menuService to know apps information.
Please let me know your views on it.
There was a problem hiding this comment.
I meant that you could create a new method in this patched class like
sortApps((apps) => {
return apps.sort((a, b) => a.name.localeCompare(b.name));
}),and then in the template just add a call to this method:
<xpath expr="<select the t-set="apps" node>" position="after">
<t t-set="apps" t-value="sortApps(apps)"/>
</xpath>since the method is really simple, everything could be in the template too, but I think it's better to keep the code where code should be (JS in this case) and let the templates handle only the rendering as much as possible.
| t-inherit-mode="extension" | ||
| owl="1" | ||
| > | ||
| <xpath expr="//header[@class='o_navbar']//t/t" position="replace"> |
There was a problem hiding this comment.
issue: The usage of @class is discouraged (see odoo/odoo@cbffd99), please use hasclass instead.
aad2db6 to
17e20fa
Compare
There was a problem hiding this comment.
Code and functional review, looks good to me.
I have just added an example for the improvement I suggested in #3199 (comment), but it's already good for merge in my opinion.
|
|
||
| onWillStart(() => { | ||
| const apps = this.menuService.getApps(); | ||
| this.sortedApps = apps.sort((a, b) => a.name.localeCompare(b.name)); |
There was a problem hiding this comment.
praise: Thanks for updating the documentation, this is something that developers rarely do in response to this kind of comments.
👍
| this.sortedApps = []; | ||
|
|
||
| onWillStart(() => { | ||
| const apps = this.menuService.getApps(); |
There was a problem hiding this comment.
I meant that you could create a new method in this patched class like
sortApps((apps) => {
return apps.sort((a, b) => a.name.localeCompare(b.name));
}),and then in the template just add a call to this method:
<xpath expr="<select the t-set="apps" node>" position="after">
<t t-set="apps" t-value="sortApps(apps)"/>
</xpath>since the method is really simple, everything could be in the template too, but I think it's better to keep the code where code should be (JS in this case) and let the templates handle only the rendering as much as possible.
| t-inherit-mode="extension" | ||
| owl="1" | ||
| > | ||
| <xpath expr="//header[@class='o_navbar']//t/t" position="replace"> |
| t-inherit-mode="extension" | ||
| owl="1" | ||
| > | ||
| <xpath expr="//header[@class='o_navbar']//t/t" position="replace"> |
web_sort_menu/__manifest__.py
Outdated
| "author": "Anusri Veerappan Prakasam, Odoo Community Association (OCA)", | ||
| "website": "https://github.com/OCA/web", | ||
| "license": "AGPL-3", | ||
| "depends": ["web", "web_responsive"], |
| t-inherit-mode="extension" | ||
| owl="1" | ||
| > | ||
| <xpath expr="//header[@class='o_navbar']//t/t" position="replace"> |
17e20fa to
bfcd7ce
Compare
|
Suggestion was super good and precise, so implemented the same. |
|
This PR has the |
Module web_sort_menu is introduced to sort apps in alphabetical order while displayed on NavBar