-
Notifications
You must be signed in to change notification settings - Fork 7.8k
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
[IMP] marketing automation: overview content #7711
Conversation
50dc69e
to
2bfe7fe
Compare
Hi @meng-odoo 👋 this PR is ready for it's first round of peer review. Let me know your thoughts, 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.
Hi @samueljlieber, you did an amazing job on this! The writing is very clear and consistent with the other documentation. I just had some small wording suggestions and formatting fixes. Please let me know if you have any questions!
541a469
to
953d287
Compare
Thank you for your very helpful review @meng-odoo! I implemented most of your suggestions in 953d287. I left the :guilabel:s out of the Targets and filters section because this doc, in particular, is just an overview of the features in the app that introducing the concepts and does not contain the active instructions on creating Targets and filters, which is handled in the I am open to discussing this as I want to make sure this doc is the best it can be! |
Hi @ksc-odoo, this PR has passed its peer review and is ready for a final content review whenever you get a chance, 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.
@samueljlieber GREAT JOB on this PR! It was very clear, concise, and easy to follow. I only have a handful of minor things that require your attention, but I'm gonna go ahead and 'Approve' this now, because I don't anticipate you having any issues making those changes.
Also, it might be worth considering adding a couple supplemental screenshots throughout this doc, if you think it would benefit the overall piece. But, that said, if you deem it unnecessary, that's totally fine, too. Just thought it was worth a quick mention for your consideration.
Once again -- awesome job -- and feel free to tag ZST when you think it's ready. 👍
52d74b1
to
22bf54a
Compare
Thank you for your helpful review @ksc-odoo! I implemented all of your suggestions in 22bf54a. @StraubCreative this PR should be good to go, 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.
Hi @samueljlieber
Nice overview doc. I think this simplifies the flow nicely and sets the groundwork for the other docs. Let's revisit it when the other docs are published.
I have a few suggestions for your review below however you can merge whenever you're ready, thanks! :)
@robodoo delegate=samueljlieber
Co-authored-by: ksc-odoo <73958186+ksc-odoo@users.noreply.github.com> Co-authored-by: meng-odoo <101904966+meng-odoo@users.noreply.github.com>
22bf54a
to
a6d7538
Compare
Thank you for your review and suggestions @StraubCreative, I've implemented them in a6d7538, and will now merge this PR :) |
@robodoo r+ |
closes odoo#7711 Signed-off-by: Samuel Lieber (sali) <sali@odoo.com> Co-authored-by: ksc-odoo <73958186+ksc-odoo@users.noreply.github.com> Co-authored-by: meng-odoo <101904966+meng-odoo@users.noreply.github.com>
This PR improves the
marketing_automation.rst
documentation by adding app specific overview content to the main TOC page and linking to the other Marketing Automation docs within the directory that go into detail each piece of content introduced in this PR.This PR can be FWP up to
master
.