[ADD] payroll: adding new contract doc #4187
Conversation
There was a problem hiding this comment.
(review for doc-structure)
Hello Lara @larm-odoo !
I have nothing to say about the toc structure, but I see you plan to add a general doc: I suggest you put it in content/applications/hr/payroll.rst and not in an overview.rst child page. Also, it seems your images still need to be compressed.
Cheers :)
Thank you Jonathan! OK, for the overview doc for Payroll, I can put it in there instead of the overview, and I will check now on the images. Does that mean the document itself was good and there were no changes? |
I didn't check your content, I'm afraid. I'm pinged for reviews of the structure when there is a structural change (such as adding a new app in the toc-tree). I left a comment, not an approval of the PR (it's an option to review without fully reviewing ^^ ) |
larm-odoo
force-pushed
the
14.0-payroll-add-new-contract-doc-larm
branch
from
1d13542 to
bd71165
Compare
There was a problem hiding this comment.
Hi @larm-odoo! Just finished my review of this doc--great job! I have some suggestions for things that could be added, plus typo/formatting/wording fixes. Please let me know if you have any questions :)
larm-odoo
force-pushed
the
14.0-payroll-add-new-contract-doc-larm
branch
from
bd71165 to
451e257
Compare
|
Thanks @meng-odoo for the review! I completed all the change requests. If I didn't comment and just marked it as 'resolved' I just made the changes. |
larm-odoo
force-pushed
the
14.0-payroll-add-new-contract-doc-larm
branch
from
451e257 to
5875349
Compare
There was a problem hiding this comment.
Hi @larm-odoo, your edits look great! I just found a few more tiny corrections to be made, especially considering the writers workshop meeting we had today :) then it should be good to go!
larm-odoo
force-pushed
the
14.0-payroll-add-new-contract-doc-larm
branch
from
5875349 to
6b98a9f
Compare
|
Thanks for the second review, @meng-odoo! I did all the edits, so I think it should be pretty good to go! |
There was a problem hiding this comment.
Looks great @larm-odoo! I found a couple more tiny corrections, but please go ahead and tag Sam for technical review after you've made them :) Good job on this!
larm-odoo
force-pushed
the
14.0-payroll-add-new-contract-doc-larm
branch
from
6b98a9f to
e8d2345
Compare
|
Hi @meng-odoo - I did all the changes, thank you so much for your reviews on this. @samueljlieber, I added you as a reviewer as well as assigned it to you- let me know if I should have just tagged you in one spot versus the other =) |
There was a problem hiding this comment.
Hi @larm-odoo 👋 I have a number of edits on this PR. Please see my suggestions below to make the changes and tag me when you're ready for me to take a second look.
Note on images: I like the images used in the PR, there are 15 images used in
the contracts.rst doc, but there are 24 images within its media folder. Please make sure you only add the images that are referenced in the doc to the media folder. These extra images need to be removed from the media folder using the git rm command 🙂
Looking forward to your changes, and please let me know if you have any questions - thank you!
larm-odoo
force-pushed
the
14.0-payroll-add-new-contract-doc-larm
branch
from
e8d2345 to
9ed39b6
Compare
There was a problem hiding this comment.
Hey @larm-odoo great job implementing the technical changes I had to the doc and handling the extra images! I had to correct myself in a few places, I am sorry about the confusion. Please see my corrections below and let me know if you have any questions with them.
Once these changes are made I can send this over for final review 👍
Thank you so much 🙂
larm-odoo
force-pushed
the
14.0-payroll-add-new-contract-doc-larm
branch
from
9ed39b6 to
a3c4e24
Compare
|
Hi @samueljlieber - all edits are done, thanks for looking at this again. If you want to check again- it's ready for you! |
There was a problem hiding this comment.
Hi @larm-odoo 👋 thank you for the final changes to this PR. I had just a couple more after taking another look. I will push those changes and push to final review 🙂
| - :guilabel:`Document`: The attached document can be replaced by clicking the | ||
| :guilabel:`✏️ (Select)` icon. A pop-up window appears so another document can be selected for | ||
| upload. The file must be a PDF. To remove the document, click the :guilabel:`🗑️ (Clear)` icon. |
There was a problem hiding this comment.
Early line break
| - :guilabel:`Document`: The attached document can be replaced by clicking the | |
| :guilabel:`✏️ (Select)` icon. A pop-up window appears so another document can be selected for | |
| upload. The file must be a PDF. To remove the document, click the :guilabel:`🗑️ (Clear)` icon. | |
| - :guilabel:`Document`: The attached document can be replaced by clicking the :guilabel:`✏️ | |
| (Select)` icon. A pop-up window appears so another document can be selected for upload. The file | |
| must be a PDF. To remove the document, click the :guilabel:`🗑️ (Clear)` icon. |
| - :guilabel:`Start Date`: The date the contract starts. Choose a date by clicking on the drop-down | ||
| menu, navigating to the correct month and year by using the :guilabel:`< > (arrow)` icons, then | ||
| clicking on the :guilabel:`date`. |
There was a problem hiding this comment.
No need for guilabel on date, again this was due to my previous (incorrect) suggestion :)
| - :guilabel:`Start Date`: The date the contract starts. Choose a date by clicking on the drop-down | |
| menu, navigating to the correct month and year by using the :guilabel:`< > (arrow)` icons, then | |
| clicking on the :guilabel:`date`. | |
| - :guilabel:`Start Date`: The date the contract starts. Choose a date by clicking on the drop-down | |
| menu, navigating to the correct month and year by using the :guilabel:`< > (arrow)` icons, then | |
| clicking on the date. |
|
|
||
| Enter the start and end dates the entry applies to. Click on the drop-down menu under | ||
| :guilabel:`From` and :guilabel:`To`, navigate to the correct month and year by using the | ||
| :guilabel:`< > (arrow)` icons, then click on the :guilabel:`date`. |
There was a problem hiding this comment.
Again no gui needed
| :guilabel:`< > (arrow)` icons, then click on the :guilabel:`date`. | |
| :guilabel:`< > (arrow)` icons, then click on the date. |
samueljlieber
force-pushed
the
14.0-payroll-add-new-contract-doc-larm
branch
from
a3c4e24 to
bc640e6
Compare
|
Implemented my technical changes in bc640e6 and pushing to @StraubCreative for final review 🙂 |
larm-odoo
force-pushed
the
14.0-payroll-add-new-contract-doc-larm
branch
from
bc640e6 to
1ca7757
Compare
There was a problem hiding this comment.
hi @larm-odoo
Doc looks good. I'm approving with comments below.
Consider addressing these please and then merge whenever you're ready (or tag me and I can look quick and merge for you, w/e you like 🙂 )
Thanks!
@robodoo delegate=larm-odoo
larm-odoo
force-pushed
the
14.0-payroll-add-new-contract-doc-larm
branch
from
1ca7757 to
0357f31
Compare
|
Hi @StraubCreative - I made the changes as requested, and updated another image that didn't have enough info on it (the detailed salary one). It doesn't look like I can merge this - it say I am not authorized above, so I think I need you to do that (if not I'd do it.) |
|
@robodoo r+ |
|
@larm-odoo staging failed: ci/runbot on 3e5e8a0b49fb1644a80713851bdc8f1c92999e2f (view more at https://runbot.odoo.com/runbot/build/52251391) |
|
@robodoo retry |
closes #4187 Signed-off-by: Lara Martini (larm) <larm@odoo.com>
Adding a new contract doc for Payroll. First I will add all the detailed docs then link them all to a general overview doc.