Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[IMP] pos_customer_display: UI Improvements #2992

Closed
wants to merge 3 commits into from
Closed

[IMP] pos_customer_display: UI Improvements #2992

wants to merge 3 commits into from

Conversation

xlu-odoo
Copy link

Prior to this commit, following recent changes integrating Bootstrap to
the customer display, the layout was lacking refinement and required a
layout overhaul. This update addresses and improves the UI.

task-3773289

I confirm I have signed the CLA and read the PR guidelines at www.odoo.com/submit-pr

@robodoo
Copy link

robodoo commented Mar 19, 2024

This PR targets the un-managed branch odoo-dev/odoo:master-pos_customer_display_standalone_app-vlst, it needs to be retargeted before it can be merged.

@xlu-odoo xlu-odoo requested a review from anso-odoo March 19, 2024 14:53
@xlu-odoo xlu-odoo force-pushed the master-pos_customer_display_standalone_app-vlst-xlu branch 3 times, most recently from e609574 to 0654976 Compare March 21, 2024 13:32
anso-odoo
anso-odoo reviewed Mar 26, 2024
View reviewed changes
Copy link

@anso-odoo anso-odoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your work, it looks really nice ! I left some comments for you 👍

addons/pos_customer_display/static/src/app/styles.scss Outdated Show resolved Hide resolved
addons/pos_customer_display/static/src/app/root.xml Outdated Show resolved Hide resolved
addons/pos_customer_display/static/src/app/root.xml Outdated Show resolved Hide resolved
@xlu-odoo xlu-odoo force-pushed the master-pos_customer_display_standalone_app-vlst-xlu branch from 0654976 to 73741ab Compare March 26, 2024 15:09
@xlu-odoo xlu-odoo requested a review from anso-odoo March 28, 2024 12:02
@xlu-odoo xlu-odoo force-pushed the master-pos_customer_display_standalone_app-vlst-xlu branch from 73741ab to 09ad5fa Compare April 9, 2024 09:27
anso-odoo
anso-odoo approved these changes Apr 9, 2024
View reviewed changes
Copy link

@anso-odoo anso-odoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left a small comment, but overall LGTM !

addons/pos_customer_display/static/src/app/root.xml Outdated Show resolved Hide resolved
@xlu-odoo xlu-odoo force-pushed the master-pos_customer_display_standalone_app-vlst-xlu branch from 09ad5fa to bd782b1 Compare April 9, 2024 12:19
@vlst-odoo vlst-odoo force-pushed the master-pos_customer_display_standalone_app-vlst branch 2 times, most recently from a5aaf5d to c63513a Compare April 10, 2024 08:49
@vlst-odoo
[REF] point_of_sale: simplify orderline data
e964880 
The function `getDisplayData` from the `Orderline` class in `models.js`
is responsible for the data that the `Orderline` component needs.
This data mostly consists of simple objects and primitives. This is
useful because it makes the object more portable, for example it can be
used with the *Broadcast Channel API*.
The problem is that some keys have been added to the object returned by
`getDisplayData` whose values are proxy objects.

In this commit we simplify the function such that all the values in the
returned object are simple objects. This is very usefull for the
folowing commit, where we actually use Broadcast Channel API.
@vlst-odoo
[ADD] pos_customer_display: create standalone app
6d32ef6 
In this pr we refactor the customer display code from pos
such that the customer display now becomes it's own standalone
owl app.

Before, the pos app would generate all the html code for the customer
display and it would send this text to the customer display window or to
the iot box. This approach was messy and wasteful, as all the data would
have to be resent for each change in the order. ( for example if the
order consisted of 10 items, each with an image, the pos would render
all the html and would send all the 10 images again for just an increase
in the quantity of one of the orderlines )

Now, the pos just sends the order details ( product names, quantities,
etc. ) and the customer display owl app handles the rendering.

This massively simplifies the code and also allows reusability of
components between the pos and the customer display.

Depending on the configuration, the pos sends the data in the following
ways:
 - Config local: broadcast channel api
 - Config remote: the pos calls a rpc method on the server and this rpc
   sends a websocket notification to the customer display
 - Config proxy: the pos sends a http request to the iot box on the
   local network; with the new implementation, the amount of data that
   is sent from the pos browser window to the iot box is immensely
   diminished. For each change in the order, the pos sends the new order
   data, but now sends aprox 15 times less data for each change.
   This is due to the fact that we now only send the actual order data (
   an object containing product names, quantities, etc ) instead of the
   full html representation of the customer display. ( as an example,
   for an order with 10 items, we now send 3kb of data instead of 47kb )

Task: 3684195
@vlst-odoo vlst-odoo force-pushed the master-pos_customer_display_standalone_app-vlst branch from c63513a to 6d32ef6 Compare April 10, 2024 09:16
@xlu-odoo xlu-odoo force-pushed the master-pos_customer_display_standalone_app-vlst-xlu branch from bd782b1 to c37319c Compare April 11, 2024 07:41
@xlu-odoo
Copy link
Author

When you have time, could you please check this one @stefanorigano

@vlst-odoo vlst-odoo force-pushed the master-pos_customer_display_standalone_app-vlst branch 9 times, most recently from 1103d42 to a9d5cbf Compare April 24, 2024 15:05
stefanorigano
stefanorigano requested changes Apr 25, 2024
View reviewed changes
Copy link

@stefanorigano stefanorigano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @xlu-odoo , thanks for you work 👍

Code is mostly ok, but I think that the design needs to be reviewed.
I wrote some remarks in the code, but it would be better to iterate a bit on Figma first and apply the design once it's confirmed.

Thanks!

addons/pos_customer_display/static/src/app/root.xml Outdated
<div class="d-flex align-items-center ps-3 pe-2 py-1 rounded-3 text-bg-dark small">Powered by <OdooLogo style="'width: 3rem;'" monochrome="true"/></div>
</div>
<div class="o_customer_display_main d-flex flex-column flex-grow-1 overflow-auto">
<div t-if="!order.finalized and order.lines?.length" class="d-flex px-4 py-1 text-bg-dark">
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To double check with the PO, but the "Your Order" bar looks unnecessary

this PR Suggestion
image image

addons/pos_customer_display/static/src/app/root.xml Outdated
<OrderWidget t-if="!order.finalized" lines="order.lines" t-slot-scope="scope" style="'background-color: #f6f6f6;'">
<Orderline line="scope.line" class="'bg-white mx-3 my-1 rounded-3' + (scope.line.isSelected ? ' selected' : '')"/>
<OrderWidget t-if="!order.finalized" lines="order.lines" t-slot-scope="scope" class="'bg-300'" style="'scroll-snap-type: y mandatory;'">
<Orderline line="scope.line" imgSize="'4rem'" infoListClasses="''" class="'o_customer_display_orderline d-flex align-items-center mx-2 my-1 ps-1 pe-3 rounded-3 bg-white fs-3' + (scope.line.isSelected ? ' selected' : '')"/>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find the overall design-line too "bulky".
At 1280x720 res, we can list 7 items only, it's not enough IMO. I think that we don't need cards...
Lightspeed and squareup are good examples of how these lists can be deeply simplified.

Current raw test
image image

@vlst-odoo vlst-odoo force-pushed the master-pos_customer_display_standalone_app-vlst branch from f5b9146 to a312fe5 Compare April 25, 2024 10:42
@vlst-odoo vlst-odoo force-pushed the master-pos_customer_display_standalone_app-vlst branch 9 times, most recently from 61c1c3f to 8eb756e Compare May 7, 2024 08:49
@xlu-odoo xlu-odoo force-pushed the master-pos_customer_display_standalone_app-vlst-xlu branch from c37319c to 6de7286 Compare May 7, 2024 11:10
@vlst-odoo vlst-odoo force-pushed the master-pos_customer_display_standalone_app-vlst branch 2 times, most recently from 7b68786 to 6132171 Compare May 7, 2024 11:35
@xlu-odoo xlu-odoo force-pushed the master-pos_customer_display_standalone_app-vlst-xlu branch from 6de7286 to 29f4579 Compare May 7, 2024 11:42
@vlst-odoo vlst-odoo force-pushed the master-pos_customer_display_standalone_app-vlst branch 12 times, most recently from 9884568 to e500258 Compare May 14, 2024 09:03
@xlu-odoo xlu-odoo force-pushed the master-pos_customer_display_standalone_app-vlst-xlu branch from 29f4579 to 7b32a38 Compare May 22, 2024 06:31
@xlu-odoo
[IMP] pos_customer_display: UI Improvements
7553468 
Prior to this commit, following recent changes integrating Bootstrap to
the customer display, the layout was lacking refinement and required a
layout overhaul. This update addresses and improves the UI.

task-3773289
@xlu-odoo xlu-odoo force-pushed the master-pos_customer_display_standalone_app-vlst-xlu branch from 7b32a38 to 7553468 Compare May 22, 2024 11:58
@fw-bot fw-bot deleted the branch master-pos_customer_display_standalone_app-vlst May 28, 2024 13:47
@fw-bot fw-bot closed this May 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stefanorigano stefanorigano stefanorigano requested changes

@anso-odoo anso-odoo anso-odoo approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
6 participants
@xlu-odoo @robodoo @stefanorigano @anso-odoo @fw-bot @vlst-odoo