Skip to content

Conversation

@vpk-odoo
Copy link
Contributor

@vpk-odoo vpk-odoo commented Jul 2, 2021

@vpk-odoo vpk-odoo added this to the 14.0 milestone Jul 2, 2021
@C3POdoo C3POdoo requested a review from a team December 9, 2021 12:27
@StraubCreative StraubCreative self-assigned this Apr 22, 2022
Copy link
Contributor

@meng-odoo meng-odoo left a comment

Choose a reason for hiding this comment

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

@StraubCreative Hi Zach! I made tons of changes to VPK's content on this doc, especially since you suggested I edit to make it shorter/clearer/more concise. Please let me know if you have any questions/comments about my changes below :) Thanks

@StraubCreative StraubCreative force-pushed the 14.0-crm-multiple-sales-teams-vpk branch from 9e9a937 to 8824706 Compare April 27, 2022 22:55
@robodoo
Copy link
Collaborator

robodoo commented Apr 27, 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 Apr 27, 2022

@StraubCreative
Copy link
Contributor

Hi @meng-odoo

Good first pass 👍
I made all of the change requests you asked for which you can see in commit 8824706

For next round of review, I highly suggest reworking the tense and tone for the whole document, namely:

  • there are a number of instances where verb tense is in continuous present, where I think it would be more ideal to write in the simple present instead (e.g. lines 56 and line 64, clicking vs. click). While continuous present is technically correct, I’d argue that this tense makes for more clunky sentence structures that don’t clearly communicate direct paths of action (guidelines reference on tense)
  • there’s a lot of 2nd person familiar pronouns in this doc (e.g. you, your)…can we clean these up a bit so it sounds less accusatory and more instructional? We’ll especially want to remove these in headings per guidelines on headings.

Looking forward to your thoughts and further improvements as you see fit!

Copy link
Contributor

@meng-odoo meng-odoo left a comment

Choose a reason for hiding this comment

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

@StraubCreative Hi Zac! Thanks for your feedback on my first review--I'll keep all of that in mind when writing my future reviews as well.

I think this one makes a lot of improvements :) Please let me know if you have any more suggestions.

@StraubCreative StraubCreative force-pushed the 14.0-crm-multiple-sales-teams-vpk branch from 8824706 to e8d2765 Compare April 29, 2022 19:59
@StraubCreative
Copy link
Contributor

@meng-odoo
Second pass changes were great! Doc sounds a lot more confident, instructional, and easier to read imo.
Please find your change requests on latest commit e8d2765 and let me know if we need anything else, otherwise we can pass over to Doc Review :)

Copy link
Contributor

@meng-odoo meng-odoo left a comment

Choose a reason for hiding this comment

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

@StraubCreative Thanks Zac! Everything looks good to me. I'll go ahead and tag Doc Review :)

@meng-odoo meng-odoo requested a review from a team May 5, 2022 00:18
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.

Perfect! 👌

@robodoo r+

@robodoo
Copy link
Collaborator

robodoo commented May 5, 2022

Because this PR has multiple commits, I need to know how to merge it:

  • merge to merge directly, using the PR as merge commit message
  • rebase-merge to rebase and merge, using the PR as merge commit message
  • rebase-ff to rebase and fast-forward

@AntoineVDV
Copy link
Collaborator

@meng-odoo @StraubCreative could you just squash the two commits into one so that the PR can be merged?

@StraubCreative StraubCreative force-pushed the 14.0-crm-multiple-sales-teams-vpk branch from e8d2765 to 85e0b9f Compare May 6, 2022 17:49
@StraubCreative
Copy link
Contributor

@meng-odoo @StraubCreative could you just squash the two commits into one so that the PR can be merged?

Should be good to go @AntoineVDV 🚀

@AntoineVDV
Copy link
Collaborator

@robodoo r+

robodoo pushed a commit that referenced this pull request May 9, 2022
closes #1056

Task: 2588786
Signed-off-by: Antoine Vandevenne (anv) <anv@odoo.com>
@robodoo robodoo closed this May 9, 2022
@robodoo robodoo temporarily deployed to merge May 9, 2022 09:26 Inactive
@fw-bot fw-bot deleted the 14.0-crm-multiple-sales-teams-vpk branch May 23, 2022 09:47
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.

6 participants