-
Notifications
You must be signed in to change notification settings - Fork 115
[IMP] website_dynamic_snippet: migrate sale order cards to Interactions class #4788
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
base: master-optimize-xml-files-loading-aans
Are you sure you want to change the base?
[IMP] website_dynamic_snippet: migrate sale order cards to Interactions class #4788
Conversation
Implementation of the website dynamic snippet to learn about the dynamic snippet. This snippet fetches product categories from the backend and displays them as a list and images.
…action refactor sale order cards to use interaction instead of public widget
…avior implement sale order cards interaction with preview behavior
add tests for sale order cards snippet
This PR targets the un-managed branch odoo-dev/odoo:master-optimize-xml-files-loading-aans, it needs to be retargeted before it can be merged. |
} | ||
|
||
async fetchOrders() { | ||
this.showConfirmOrders = this.el.dataset.showConfirmOrders === "true" ?? 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.
?? false
- this part is not required
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.
As we have discussed earlier, if not set the falsy value explicitly then it would result in undefined
|
||
async fetchOrders() { | ||
this.showConfirmOrders = this.el.dataset.showConfirmOrders === "true" ?? false; | ||
this.noOfOrders = parseInt(this.el.dataset.noOfOrders); |
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.
Adding a fallback number here would be nice
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.
Earlier, it was there, you had only mentioned it, there is no need, as data-no-of-orders
attribute is set by default
run: function () { | ||
return this.anchor.childElementCount === 30; | ||
}, |
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 comparing like this in not enough to make this step fail.
You have to explicitly throw an error here if count not equal to 30.
Also, why not use the :nth-child
selector? With that you can get rid of the run function, since the last step is ideally just a check anyway.
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.
Noted!
from odoo.tests import HttpCase, tagged | ||
|
||
|
||
@tagged("post_install", "-at_install", "sale_order_cards") |
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's okay to put tags like this when you are testing - but this is not required, you can remove the "sale_order_cards" tag.
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.
Ok
<!-- Cards will be rendered here --> | ||
</div> | ||
<div class="d-flex justify-content-center my-4"> | ||
<button class="btn btn-primary" id="load_more_orders">Load More Orders...</button> |
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.
You should hide this button by default and show only when there could be potentially more orders to load.
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.
if (temp_orders.length === 0) {
const loadMoreButton = this.el.querySelector("#load_more_orders");
loadMoreButton.classList.add("d-none");
}
it is already present
refactored the sale order cards snippet in website_dynamic_snippet to use
the interactions class instead of the legacy public widget. Also introduced tours
for testing this snippet thoroughly.