-
Notifications
You must be signed in to change notification settings - Fork 9k
[IMP] PLM: Update style in ECO #12752
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
slinkous
commented
Apr 1, 2025
•
edited
Loading
edited
- Match updated style guide
- Remove annotation from images
- Remove "tablet view" content
f259b8b
to
fa81d26
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.
Looks good, @slinkous ! After addressing my comments, since it's a 1 pt PR, feel free to tag Sam after to get it merged :D
content/applications/inventory_and_mrp/plm/manage_changes/engineering_change_orders.rst
Outdated
Show resolved
Hide resolved
@@ -251,21 +262,20 @@ latest |ECO|. | |||
:guilabel:`Revision` smart button of the |ECO|. | |||
|
|||
.. image:: engineering_change_orders/bom-version.png | |||
:align: center | |||
:alt: View current *BOM* version in the Miscellaneous tab. | |||
:alt: View current *BOM* version in the *Miscellaneous* tab. | |||
|
|||
.. _plm/eco/tablet-view: | |||
|
|||
Create ECO from tablet view |
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.
Alrighty, so it looks like this entire section became outdated after version 17.0 when tablet view was replaced with Shop Floor 😢. To keep this PR focused on the superficial updates for PLM, let’s go ahead and remove this section (up until "View ECO") for now. (Actually, for View ECO, let's bring the heading up by 1 because I don't think it should be nested within the tablet view heading)
No worries though—this gives us a great opportunity to dive into Shop Floor in a follow-up PR! I’ll walk you through how it works, including how to create an ECO from the Shop Floor module, and we can add those updated instructions to this doc later. Sounds like a plan? 😊
Thanks for understanding, and let me know if you have any questions about this change!
fa81d26
to
bac61b9
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.
we're super close! looks like the build error is caused by the mention of tablet view in line 18. Let's also delete the TODO at the bottom of the doc before merge :D
content/applications/inventory_and_mrp/plm/manage_changes/engineering_change_orders.rst
Show resolved
Hide resolved
content/applications/inventory_and_mrp/plm/manage_changes/engineering_change_orders.rst
Show resolved
Hide resolved
content/applications/inventory_and_mrp/plm/manage_changes/engineering_change_orders.rst
Outdated
Show resolved
Hide resolved
3aaba28
to
254b175
Compare
@samueljlieber ready for merge |
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! Thanks for your work on this PR.
I am requesting changes as there are a number of remaining changes that need to be made to update the style of this doc, as well as I want to double check on @Felicious comment here.
let’s go ahead and remove this section (up until "View ECO") for now
However, in the latest commit the View ECO section is removed. Is this intended?
Please run make review
on the image folder for this doc (content/applications/inventory_and_mrp/plm/manage_changes/engineering_change_orders/
) to locate and remove the image files that are no longer used in the doc.
There is also an emoji that need to be replaced with an :icon:
.
Please tag me for another look once you've had a chance to address these! Thank you :)
content/applications/inventory_and_mrp/plm/manage_changes/engineering_change_orders.rst
Outdated
Show resolved
Hide resolved
content/applications/inventory_and_mrp/plm/manage_changes/engineering_change_orders.rst
Outdated
Show resolved
Hide resolved
Hi @samueljlieber! Thanks for catching that! 😊 As far as I know, the View ECO section is still relevant and shouldn't be removed unless the UI has changed significantly. Since it’s currently an H3 heading, let’s change it to H2 after removing the section above to maintain the structure. |
dfsdf dfsdf Fix unfinished sentence Remove tablet link Apply suggestions from code review Co-authored-by: Sam Lieber (sali) <[email protected]> Restore view
8708da6
to
7eeb3c3
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.