-
Notifications
You must be signed in to change notification settings - Fork 9.8k
[ADD] barcode: operations and commands barcode printing #13355
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
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.
Great work so far, @slinkous ! 😊 The doc is really in-depth and it's going to be SO helpful! 😍
I know you haven't requested a review from me, yet! I just did a quick scan 👀 😉 to catch some quick formatting and style things. I presumed that since you pushed this PR up at the end of the day, you'll still have it open when you come in tomorrow, so it'll be easy to do some fixes before you move onto another task 😊
content/applications/inventory_and_mrp/barcode/setup/operation_types.rst
Outdated
Show resolved
Hide resolved
content/applications/inventory_and_mrp/barcode/setup/operation_types.rst
Outdated
Show resolved
Hide resolved
content/applications/inventory_and_mrp/barcode/setup/operation_types.rst
Outdated
Show resolved
Hide resolved
content/applications/inventory_and_mrp/barcode/setup/operation_types.rst
Outdated
Show resolved
Hide resolved
content/applications/inventory_and_mrp/barcode/setup/operation_types.rst
Outdated
Show resolved
Hide resolved
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.
super minor nitpicks! planning to send this to tech review next so i'd like to take a look before then (:
but wonderful job!!! really helpful & comprehensive doc
content/applications/inventory_and_mrp/barcode/setup/operation_types.rst
Outdated
Show resolved
Hide resolved
content/applications/inventory_and_mrp/barcode/setup/operation_types.rst
Outdated
Show resolved
Hide resolved
content/applications/inventory_and_mrp/barcode/setup/operation_types.rst
Show resolved
Hide resolved
content/applications/inventory_and_mrp/barcode/setup/operation_types.rst
Outdated
Show resolved
Hide resolved
content/applications/inventory_and_mrp/barcode/setup/operation_types.rst
Outdated
Show resolved
Hide resolved
c39a65e
to
d93f642
Compare
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.
Great doc with so much detail!! @slinkous Proud of your work 😊
I just really minor things to add, and you're getting more and more familiar with our style and conventions 😊
Starting from the next doc you push up, you'll be officially joining the other writers in our normal peer review process! Tag Jess or Lara in peer review, me in final, and then Sam in technical!
content/applications/inventory_and_mrp/barcode/setup/operation_types.rst
Show resolved
Hide resolved
content/applications/inventory_and_mrp/barcode/setup/operation_types.rst
Outdated
Show resolved
Hide resolved
content/applications/inventory_and_mrp/barcode/setup/operation_types.rst
Outdated
Show resolved
Hide resolved
content/applications/inventory_and_mrp/barcode/setup/operation_types.rst
Outdated
Show resolved
Hide resolved
content/applications/inventory_and_mrp/barcode/setup/operation_types.rst
Outdated
Show resolved
Hide resolved
content/applications/inventory_and_mrp/barcode/setup/operation_types.rst
Outdated
Show resolved
Hide resolved
content/applications/inventory_and_mrp/barcode/setup/operation_types.rst
Outdated
Show resolved
Hide resolved
content/applications/inventory_and_mrp/barcode/setup/operation_types.rst
Outdated
Show resolved
Hide resolved
content/applications/inventory_and_mrp/barcode/setup/operation_types.rst
Outdated
Show resolved
Hide resolved
content/applications/inventory_and_mrp/barcode/setup/operation_types.rst
Outdated
Show resolved
Hide resolved
67e5404
to
93694be
Compare
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.
@samueljlieber this is ready for review, and much appreciated!
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.
Hi @slinkous! Before I review would you mind resolving the merge conflict? Thank you!
4aef40b
to
26fb546
Compare
@samueljlieber conflict resolved. I swear that other change hadn't gone through until after I tagged you. 😅 Thanks for video guide! |
No worries @slinkous, thank you for taking care of it! Review coming soon! |
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.
Hi @slinkous! Awesome work on this Barcode PR! The doc is really in-depth and I like the amount of reference links you used to add more additional context.
Im requesting changes just to address and discuss a couple items, all pretty minor :)
Nitpicks- please correct the lines over 100 chars and early line breaks (can use make review
to find these).
Tag me for one more quick look once you've had a moment to address these!
Thank you!
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.
Please remove this unused image :)
(content/applications/inventory_and_mrp/barcode/setup/operation_types/print-picking-inventory.png
)
content/applications/inventory_and_mrp/barcode/setup/operation_types.rst
Outdated
Show resolved
Hide resolved
content/applications/inventory_and_mrp/barcode/setup/operation_types.rst
Outdated
Show resolved
Hide resolved
content/applications/inventory_and_mrp/barcode/setup/operation_types.rst
Outdated
Show resolved
Hide resolved
content/applications/inventory_and_mrp/barcode/setup/operation_types.rst
Outdated
Show resolved
Hide resolved
content/applications/inventory_and_mrp/barcode/setup/operation_types.rst
Outdated
Show resolved
Hide resolved
content/applications/inventory_and_mrp/barcode/setup/operation_types.rst
Outdated
Show resolved
Hide resolved
barcode if there is one available, or tap the :icon:`fa-cog` :guilabel:`(cog)` icon to open the | ||
Barcode Actions menu and tap the :guilabel:`Print Picking Operation` 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.
Please use the "gear" label for this icon over "cog" :)
Bold Barcode app name and lowercase actions menu
barcode if there is one available, or tap the :icon:`fa-cog` :guilabel:`(cog)` icon to open the | |
Barcode Actions menu and tap the :guilabel:`Print Picking Operation` button. | |
barcode if there is one available, or tap the :icon:`fa-cog` :guilabel:`(gear)` icon to open the | |
**Barcode** actions menu and tap the :guilabel:`Print Picking Operation` 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.
But otherwise the rule of explicit label > hover label > fontAwesome name applies? This one's just an old convention?
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, please see our Vale linter rule here: https://github.com/Felicious/odoo-vale-linter/blob/7b0e3b245ba4e4a8fb09d2c7345320979e3ef235/styles/Odoo/Icons.yml#L61
Operations | ||
---------- | ||
|
||
- :guilabel:`Receipts (WHIN)` creates a new order to receive products into inventory. |
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.
Continuing from my prev comment, these are printed so no :guilabel: IMO, and could use literal for the codes (optional)
- :guilabel:`Receipts (WHIN)` creates a new order to receive products into inventory. | |
- **Receipts** (`WHIN`) creates a new order to receive products into inventory. |
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.
Okay, I like this solution! I ran through italics/bold/literal for the whole phrase, but the parentheses are my addition as well. I'm going to convert it all to **Operation Name**
\
BARCODE``, because it looks the most like the actual printed barcodes, and the barcodes themselves seem to all stick to monospace fonts.
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 can't escape the backticks. I think you'll get what I mean though.
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! I agree :)
**Operation Name** `BARCODE`
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.
content/applications/inventory_and_mrp/barcode/setup/operation_types.rst
Outdated
Show resolved
Hide resolved
26fb546
to
fcf2f00
Compare
@samueljlieber This is ready for re-review. Tysm for formatting help! |
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.
Hi @slinkous, thank you for addressing most of my comments– however there are still a few left to resolve.
Please remove this unused image: #13355 (comment)
Please tag me once more once addressed. Thank you!
content/applications/inventory_and_mrp/barcode/setup/operation_types.rst
Outdated
Show resolved
Hide resolved
content/applications/inventory_and_mrp/barcode/setup/operation_types.rst
Outdated
Show resolved
Hide resolved
fcf2f00
to
92641da
Compare
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.
content/applications/inventory_and_mrp/barcode/setup/operation_types.rst
Outdated
Show resolved
Hide resolved
282efa0
to
f5ed712
Compare
@robodoo r+ |
@slinkous you may want to rebuild or fix this PR as it has failed CI. |
Co-authored-by: Felicious <feku@odoo.com>
f5ed712
to
bbd7f5a
Compare
@robodoo r+ |
No description provided.