Skip to content

Conversation

@StraubCreative
Copy link
Contributor

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:

  • Digest Emails (Overview)
    • Customize Your Odoo Periodic Digest
    • Create your own Digest Email*

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.

@robodoo
Copy link
Collaborator

robodoo commented Dec 21, 2021

@AntoineVDV AntoineVDV removed the request for review from a team December 22, 2021 08:22
@AntoineVDV
Copy link
Collaborator

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.

Copy link
Contributor

@jcs-odoo jcs-odoo left a 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.

@StraubCreative StraubCreative force-pushed the 15.0-general-add-digest-emails-documentation-zst branch from e47365d to 5c74ea6 Compare January 25, 2022 19:44
@StraubCreative StraubCreative force-pushed the 15.0-general-add-digest-emails-documentation-zst branch from 5c74ea6 to 6ffe57c Compare January 25, 2022 20:02
@StraubCreative
Copy link
Contributor Author

Hi @jcs-odoo !

Here's a 2nd pass @ 6ffe57c where I've added in all of your suggestions, namely:

  • Squashed 3 RST files into one --> digest_emails.rst
  • Reformatted doc to break at or before 100th character
  • Added internal hyperlink anchors where applicable
  • Restored sidebar navigation order as it was prior before alphabetizing
  • Resized images to 1080p instead of 4k HD
  • Content changes to headings and copy per your notes

Looking forward to your review, thanks!

cc: @jubodoo

@StraubCreative
Copy link
Contributor Author

@jcs-odoo hi there! Checking to see if you had a chance to review the most recent push after your change requests.
Thanks and please let me know 🙏

cc: @jubodoo @AntoineVDV

Copy link
Contributor

@jcs-odoo jcs-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 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.

@StraubCreative StraubCreative force-pushed the 15.0-general-add-digest-emails-documentation-zst branch 2 times, most recently from 8b8280b to a7ac065 Compare March 2, 2022 22:59
@StraubCreative StraubCreative requested review from a team and jcs-odoo March 2, 2022 23:01
@StraubCreative
Copy link
Contributor Author

@jcs-odoo

Thank you for the review today.
Here's the latest batch of changes as reflected on a7ac065:

  • fixed remaining 100th character breaks
  • new screenshots for periodic-digest.png and digest-email-settings.png, taken at 110% zoom, sized to 1080p + compressed with pngquant
  • note indentation fixed
  • typo at line 36 fixed
  • rephrased lines 36-38 per suggestion
  • removed future tense from doc, changed to present
  • Removed “create” from heading at line 54, added blank line after all headings in doc
  • line 74, number to word change
  • removed ordinal language and semicolons in ordered list starting on line 78

Please let me know if there's anything else or if we're good to merge, thanks again! 🙏

cc: @jubodoo

Copy link
Contributor

@jcs-odoo jcs-odoo left a 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.

    • you can go to the line everywhere that there is a space. So that can be "in the middle" of a menuselection or a bold text.
    • In my suggestion at lines 7-12, GitHub displayed it as follow, but even if "high-level" appears on another line, it's still part of line 7
      image
  • 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 :)

@StraubCreative StraubCreative force-pushed the 15.0-general-add-digest-emails-documentation-zst branch from a7ac065 to bab2a36 Compare March 11, 2022 19:04
@StraubCreative
Copy link
Contributor Author

@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.
Thanks again!

@StraubCreative StraubCreative requested a review from a team March 11, 2022 19:12
Copy link
Collaborator

@AntoineVDV AntoineVDV left a 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).

@StraubCreative StraubCreative force-pushed the 15.0-general-add-digest-emails-documentation-zst branch from bab2a36 to 052034f Compare March 18, 2022 20:04
@robodoo
Copy link
Collaborator

robodoo commented Mar 18, 2022

Sorry, I didn't know about this PR and had to retrieve its information, you may have to re-approve it.

@robodoo
Copy link
Collaborator

robodoo commented Mar 18, 2022

@StraubCreative
Copy link
Contributor Author

@AntoineVDV thanks for your feedback 🙏
Went ahead and committed your suggestions which can be seen on 052034f.

  • Removed unnecessary show-toc metadata on line 1
  • Removed "->" arrow on/near line 56
  • Added backticks (`) to technical values in ref table.
  • ALSO: fixed incorrect Value for New Leads Label in ref table (was a duplicate Value of row 2 before).

Please let me know if there are any other items you'd like me to address.
Thanks again for all of your help. Excited!

Copy link
Collaborator

@AntoineVDV AntoineVDV left a comment

Choose a reason for hiding this comment

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

LGTM!

@robodoo r+

@jcs-odoo
Copy link
Contributor

👌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants