Skip to content

Conversation

ksc-odoo
Copy link
Contributor

@ksc-odoo ksc-odoo commented Aug 2, 2022

Task ID: 2942081

@robodoo
Copy link
Collaborator

robodoo commented Aug 2, 2022

@ksc-odoo ksc-odoo requested a review from mivu-odoo August 2, 2022 17:30
@ksc-odoo
Copy link
Contributor Author

ksc-odoo commented Aug 2, 2022

@mivu-odoo hey there!

Added a "Getting Started" section (and Getting Started documentation sections) to the Marketing Automation documentation.

Once I get your approval, I'll be sure to send this along to Zac.

Thanks!

@C3POdoo C3POdoo requested a review from a team August 2, 2022 17:31
@mivu-odoo mivu-odoo force-pushed the 14.0-marketing-automation-first-campaign-ksc branch from df60c81 to fe684fd Compare August 31, 2022 23:07
@mivu-odoo
Copy link
Contributor

Hello @ksc-odoo! I pushed commit fe684fd. In this commit, I updated the RST file names for getting_started.rst, first_campaign.rst, target_audience.rst, testing_running.rst, and workflow_activities.rst.

Before, all the RST files in this PR used hyphens instead of underscores for spaces. RST file names should use underscores for spaces. It can get confusing because image files names use hyphens for spaces.

I'll have to work on this another day to fix the No "build succeeded." found in logs. message; this PR is still failing the ci/documentation code check.

@mivu-odoo mivu-odoo force-pushed the 14.0-marketing-automation-first-campaign-ksc branch from fe684fd to 1e30c64 Compare August 31, 2022 23:24
@mivu-odoo
Copy link
Contributor

Adding on commit 1e30c64

In this commit, I...

  • changed the code line in marketing_automation.rst to reference the new RST file name marketing_automation/getting_started (with the underscore, not the hyphen)
  • renamed the image folder to first_campaign (with the underscore) for first_campaign.rst
  • updated the image folder paths referenced in first_campaign.rst
  • renamed the image folder to target_audience (with the underscore) for target_audience.rst
  • updated the image folder paths referenced in target_audience.rst

Will need to push another commit to fix the image folder names and references for testing_running.rst and workflow_activities.rst

@mivu-odoo mivu-odoo force-pushed the 14.0-marketing-automation-first-campaign-ksc branch from 1e30c64 to 2b8ac51 Compare September 1, 2022 19:56
@mivu-odoo
Copy link
Contributor

Adding on commit 2b8ac51

In this commit, I...

  • renamed the image folder to testing_running (with the underscore) for testing_running.rst
  • updated the image folder paths referenced in testing_running.rst
  • renamed the image folder to workflow_activities (with the underscore) for workflow_activities.rst
  • updated the image folder paths referenced in workflow_activities.rst

@mivu-odoo mivu-odoo force-pushed the 14.0-marketing-automation-first-campaign-ksc branch from 2b8ac51 to b5ed0a7 Compare September 1, 2022 21:06
@mivu-odoo
Copy link
Contributor

mivu-odoo commented Sep 1, 2022

Latest commit b5ed0a7 - the build errors were corrected and the PR is now passing all the checks 🥳

For reference, the fixes in this commit were:

  • fixed the file path references from getting-started/[rst_file_name].rst to getting_started/[rst_file_name].rst in the toctree file getting_started.rst
  • fixed indentation on bulleted list items in first_campaign.rst and workflow_activities.rst; if the text of a bullet list item runs over onto a second line, make sure the first letter of the second line aligns with the first letter of the first line. Also, you don't have to put blank lines between each list item in a bulleted list.
  • shortened the :alt: text on the image directive for filters7.png in target_audience.rst from two lines to one line. I tried to just align the first letters of both lines, but it was still giving me a build error, so I just shortened the text to one line.

Copy link
Contributor

@mivu-odoo mivu-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 @ksc-odoo!

Thank you for your hard work and patience on these three user docs for marketing automation!

I have finished my final review. I found some edits that could be made for concision. I also found some edits that should be made to avoid the use of second-person pronouns (you, yours) and for proper :guilabel: use (if it's a button or field name, it should have a :guilabel:).

I also renamed the filters image files in target_audience to be more descriptive (filters-opportunities.png versus filters6.png)

Let me know if you have any questions, or if you would like ZST or me to walk you through how to make edits on an existing PR and squash your commits. When the revisions are finished, please tag me again for a final look. Thank you 😸

@ksc-odoo ksc-odoo force-pushed the 14.0-marketing-automation-first-campaign-ksc branch 3 times, most recently from 2e769bf to 702c281 Compare October 4, 2022 20:44
@ksc-odoo ksc-odoo requested a review from mivu-odoo October 4, 2022 20:44
Copy link
Contributor

@mivu-odoo mivu-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 @ksc-odoo!

Thank you for the quick turnaround on this PR! I have a couple more edits that I must've missed on my first round (mainly on the workflow_activities.rst file).

I also noticed the favorite-filter-field.png image file is still in the target_audience folder. We can try to delete it in this round of revisions. So, the process would look something like this:

  • Open the Git CMD terminal
  • cd github/odoo/documentation
  • git checkout 14.0
  • git pull
  • git checkout 14.0-marketing-automation-first-campaign-ksc
  • Navigate to the target_audience image folder
  • Right-click on the favorite-filter-field.png file
  • Select "Copy as a path"
  • On the terminal, run git rm image-file-path (you can just hit "CTRL" + "V" to paste the image file path into the terminal)
  • Make changes on the RST file based on the review comments
  • git add .
  • git commit -m "type whatever commit message"
  • git rebase -i HEAD~2
  • proceed to squash commits > checkout to the base branch 14.0 and run a fresh pull > checkout back to the feature branch > git rebase 14.0 > git push origin +14.0-marketing-automation-first-campaign-ksc

Let me know if you run into any issues or need clarification on any of the comments. When you're ready, please re-request my review so I can take another look. Thank you 😸

@ksc-odoo ksc-odoo force-pushed the 14.0-marketing-automation-first-campaign-ksc branch from 702c281 to 3b81d4b Compare October 5, 2022 16:45
@ksc-odoo ksc-odoo requested a review from mivu-odoo October 5, 2022 16:45
Copy link
Contributor

@mivu-odoo mivu-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 @ksc-odoo! Thank you for the quick revisions! I just had three more edits, but we're almost there!

Once you make those changes, please tag me for a final look and then we can ship this off to ZST for final technical review. Thank you 😸

@ksc-odoo ksc-odoo force-pushed the 14.0-marketing-automation-first-campaign-ksc branch from 3b81d4b to fc30dd4 Compare October 5, 2022 21:14
@ksc-odoo ksc-odoo requested a review from mivu-odoo October 5, 2022 21:15
@mivu-odoo mivu-odoo force-pushed the 14.0-marketing-automation-first-campaign-ksc branch from fc30dd4 to 0e7be9d Compare October 5, 2022 22:20
Copy link
Contributor

@mivu-odoo mivu-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 @ksc-odoo!

Thank you! I went ahead and pushed commit 0e7be9d to fix a small error (there was an extra backtick that didn't need to be there in one sentence). Since that's out of the way, this is good to go to ZST for final technical review. I'll tag him for you 😸

@mivu-odoo
Copy link
Contributor

Hi @StraubCreative this is ready for final technical review!

Copy link
Contributor

@StraubCreative StraubCreative left a comment

Choose a reason for hiding this comment

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

Pausing my review here. Please address:

  • image widths as listed in original comment here. A lot of the screenshots are too wide or might not be in HD so let's try to a) get them down to tablet breakpoint or so by resizing the browser and b) cut whitespace out of the screenshot by using both a 120%+ zoom and smaller browser window. For convenience I've attached a number of screenshot replacements in the CR comments.
  • trailing whitespaces-- there's too many of them
  • content suggestions where indicated

When these are addressed across all docs/images I'll resume review.
Thanks!

cc: @mivu-odoo

@ksc-odoo ksc-odoo force-pushed the 14.0-marketing-automation-first-campaign-ksc branch from 175c5cf to 7b83eae Compare November 21, 2022 19:21
Copy link
Contributor

@StraubCreative StraubCreative left a comment

Choose a reason for hiding this comment

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

Hi @ksc-odoo

Thanks for the quick revisions last round. Here are some more below to complete the review.

I'd invite you to regularly review the content and RST guidelines, as a number of the CRs listed on my prior and current reviews are covered in those documents. With regular review, it should help move PRs along faster and move you one step closer to PR king 👑

I think if all of these items below are addressed in their entirety, then we'll be good to push to DR. It'll be really great to get some Marketing docs live!

Thanks and do let me know if you have any questions.

P.S. If you're using VSC for your text editor, and don't have it already, remind me to give you the 100th character script Sam made...it's very helpful for clipping lines at or before 100th character 👍

@ksc-odoo ksc-odoo force-pushed the 14.0-marketing-automation-first-campaign-ksc branch from 7b83eae to 14a5d1b Compare November 30, 2022 00:08
@ksc-odoo ksc-odoo force-pushed the 14.0-marketing-automation-first-campaign-ksc branch from 14a5d1b to 2f89d54 Compare December 2, 2022 23:05
@ksc-odoo
Copy link
Contributor Author

ksc-odoo commented Dec 2, 2022

@StraubCreative Alright - did my best to clean up the advanced doc (understanding_metrics). It's ready for your review. Ready to make any necessary changes. Thanks!

@StraubCreative StraubCreative force-pushed the 14.0-marketing-automation-first-campaign-ksc branch from 2f89d54 to 764d3b3 Compare December 6, 2022 03:27
Copy link
Contributor

@StraubCreative StraubCreative left a comment

Choose a reason for hiding this comment

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

Hi @ksc-odoo 👋

Thank you for your efforts on this. I think we're at the finish line 🚀

The comments below are mostly for instructional purposes or for further consideration, as I've cleaned up the remaining technical issues that were still affecting the PR on the latest commit 764d3b3, which include:

  • trailing whitespaces / line breaks
  • 100th character count
  • screenshots being too wide or not in HD
  • failing CI checks

For comments related to content on these docs, we can address on a future PR, but for now it's more important to get these live than to keep them sitting in review 😉

Again, good effort! Passing on to DR...

in numbers.
| Let’s consider the example below:
Consider the following example:
Copy link
Contributor

Choose a reason for hiding this comment

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

For this document, I'll be logging a mix of items to work on for later, as well as items I will fix right away to bring the technical requirements up to standard so it can pass review.

Starting here, the written context should not rely on imagery to communicate information. It leaves too much assumption up to the viewer (especially if they're unfamiliar with the UI) and prevents users with low internet connections from absorbing the content.

So for this document, I'd break all instances of that symbiotic relationship. The standard for technical/documentation writing is that screenshots and other media are supplemental to the written content, as in, the written word should be able to stand on it's own in clear, concise language without images to rely on.

As well, writing an entire article off of one use case guarantees that we're alienating readers in which the use case does not apply. Technical documentation ideally is written in a way where no matter who reads it, every reader leaves the article with clear and concise knowledge on how a specific feature works for their own projects, not so much how the feature works in one specific scenario.

@StraubCreative StraubCreative force-pushed the 14.0-marketing-automation-first-campaign-ksc branch from 764d3b3 to 53b0c86 Compare December 6, 2022 03:46
@StraubCreative
Copy link
Contributor

Follow up from previous comment:

53b0c86: fixed alt texts on understanding_metrics.rst

Doc should be up to technical standards now. All checks pass 👍

@StraubCreative StraubCreative requested a review from a team December 6, 2022 03:49
@StraubCreative
Copy link
Contributor

This PR for marketing automation is ready for your review @odoo/doc-review 🙏

Copy link
Collaborator

@Feyensv Feyensv left a comment

Choose a reason for hiding this comment

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

Some little comments ;)

@StraubCreative StraubCreative force-pushed the 14.0-marketing-automation-first-campaign-ksc branch from 53b0c86 to ee3a180 Compare December 6, 2022 19:10
@StraubCreative
Copy link
Contributor

Hi @Feyensv
Thank you for your review.
I believe I addressed all of your your change requests on ee3a180.
Please take another look when you have a moment 🙏

Copy link
Collaborator

@Feyensv Feyensv left a comment

Choose a reason for hiding this comment

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

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