-
Notifications
You must be signed in to change notification settings - Fork 10.7k
[ADD] general/digest_emails: add new digest emails docs #1446
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
[ADD] general/digest_emails: add new digest emails docs #1446
Conversation
|
Hi, @StraubCreative no need to manually assign reviewers. We now have a "codeowners" feature to do that automatically ;) Codeowners only request a review from @odoo/doc-review when they all submitted an approving review. |
jcs-odoo
left a comment
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.
Hello @StraubCreative !
That's a nice first one on this repo, I must say! :)
You used plenty of commands specific to this documentation repo.
I added some guideline comments and suggestions to achieve a better result, but it's mainly about the structure and RST rather than content.
Structure-wise:
- Fabien asked us not to put "important" content in the category pages, contrary to what we started to do. That is why you see a few pages in the "misc" section with content in the category pages. It turns out it is not a good example anymore. (sadly. I preferred it this way)
- I think the content of this PR could easily be "squashed" on a single page.
- This would solve the point above, about the category page, since it wouldn't be a category page anymore
- It also solves the issue about the titles (see comment in the content itself)
- It also helps achieve a better structure inside each page(see comment in the content itself)
- from a content perspective, it makes sense to split into various pages if the topics are quite different, but here it is not, imo.
- Customization and Creation aren't different topics. They both are about the same kind of entries and data fields
- However, custom KPI with Odoo Studio is a different topic
So, I would suggest squashing everything you explain in a single doc. To know what kind of content to explain where and how, please refer to the content guidelines: structure section.
Remember that for each heading, there is first a "what it is" explanation (if needed), then a "how to do it."
Here is a suggestion for the structure. I can be different, of course, but maybe this can help you.
- H1: Digest Emails
- "what" explanation of what they are (as you did in digest_emails.rst)
- H2: Configuration
- How to enable/disable the feature (still what's in digest_emails.rst)
- then how to configure a mail digest (which squashes your "create" and "customize). it could be a different heading, maybe
- H3: Custom KPIs with Studio
- H2: ? user side's usage? (probably not necessary, though)
RST wise:
- break the line at 100th character
- make sure to leave a white line between each command. Headings and paragraphs are different commands, so a white line is required between them too.
- see comments in the content
Now, you also mention that the KPI part is still a WIP. Do you intend to update this PR before merging it? If so, you can put this PR as a draft until ready for merging-reviewing. But maybe you plan to merge this first and improve the content later? Then it's fine like it is now.
It's already a lot of feedback, but I haven't read the content itself in-depth, so there's not much feedback about it.
Good luck! And let me know if something needs more clarification.
content/applications/general/digest_emails/create_digest_email.rst
Outdated
Show resolved
Hide resolved
content/applications/general/digest_emails/create_digest_email.rst
Outdated
Show resolved
Hide resolved
content/applications/general/digest_emails/create_digest_email.rst
Outdated
Show resolved
Hide resolved
content/applications/general/digest_emails/customize_periodic_digest.rst
Outdated
Show resolved
Hide resolved
content/applications/general/digest_emails/customize_periodic_digest.rst
Outdated
Show resolved
Hide resolved
e47365d to
5c74ea6
Compare
5c74ea6 to
6ffe57c
Compare
|
Hi @jcs-odoo ! Here's a 2nd pass @ 6ffe57c where I've added in all of your suggestions, namely:
Looking forward to your review, thanks! cc: @jubodoo |
|
@jcs-odoo hi there! Checking to see if you had a chance to review the most recent push after your change requests. cc: @jubodoo @AntoineVDV |
jcs-odoo
left a comment
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.
Thanks for all the work, Zach.
Here are still a few comments. Some are just ideas.
I skim-read the second half as I have to go, but overall, it's good.
- images
- ideally take the screenshots with an HD resolution screen, not more. periodic-digest.png is too large.
- periodic-digest.png looks a bit weird
- digest-email-settings.png looks like taken on an older version than 15.0 (or the colors are washed out somehow) (nitpicking :) )
- please compress your pngs with pngquant https://www.odoo.com/documentation/saas-15.1/contributing/documentation.html#contributing-pngquant
8b8280b to
a7ac065
Compare
|
Thank you for the review today.
Please let me know if there's anything else or if we're good to merge, thanks again! 🙏 cc: @jubodoo |
jcs-odoo
left a comment
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 @StraubCreative :)
Thanks: here are some last comments, but I have already approved the PR.
Since I approved it, you can ask for a review from @odoo/doc-review . They're the team that checks the PR's technical part and then merges it. So, with my reviews, it should help you get your PR approved by them ;)
-
the line break still isn't right.
-
about the images, I didn't mean that they all have to be 1080 pixels wide. Apologies :) I meant on an HD (1080p) screen. That gives a good size for the documentation. But it works well like that now, so no worries.
-
Usually, please click on "resolve conversation" yourself if you agree with the last comment and that you applied it.
Cheers :)
a7ac065 to
bab2a36
Compare
|
@jcs-odoo thanks again for all of your feedback on this PR 🙏 I went ahead and applied your last suggestions for 100th character cleanup on lines 7, 10, 36, and 62 as seen on bab2a36. In all honesty it might take me a few more tries to get right but I appreciate the guidance around these little nuances-- for example on line 7 my text editor says that's 101 characters but it seems like you're saying not to count markup symbols like *, `, etc. in the character count? In any case, think we're ready for technical review. |
AntoineVDV
left a comment
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 @StraubCreative, please find my review comments below :)
my text editor says that's 101 characters but it seems like you're saying not to count markup symbols like *, `, etc. in the character count?
Every single character counts. Make sure your editor is a proper text editor (not a Word-like soft) and that it's properly configured (it should be the case by default).
bab2a36 to
052034f
Compare
|
Sorry, I didn't know about this PR and had to retrieve its information, you may have to re-approve it. |
|
@AntoineVDV thanks for your feedback 🙏
Please let me know if there are any other items you'd like me to address. |
AntoineVDV
left a comment
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.
LGTM!
@robodoo r+
|
👌 |

Overview
Project Task here
Requested by @LNAodoo
Summary of changes
This PR adds a new Digest Emails section inside General / Miscellaneous, and includes 3 articles nested as follows:
I also alphabetized the Miscellaneous TOC menu in the sidebar to make it easier to navigate so tagging you @jcs-odoo to review.
(*)Note: the third article is a work in progress however what's there now was what was on the previous info tab inside Periodic Digest settings (screenshot here showing old layout). Will continue to work on this with feedback from UX team.