-
Notifications
You must be signed in to change notification settings - Fork 10.9k
[IMP] whatsapp: clarify "to" field for test message recipients, remove future tense #15451
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
Conversation
|
Hi @larm-odoo! This PR is ready for review |
larm-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.
WOW - what a BEAST of a doc!!!
First, remember to upate the format of the title for this PR. This is not an ADD as it is an improvement We have a format for the titles, so this should be:
[IMP] WhatsApp: Add "to" field and remove future tense
There's a lot of suggestions I made. Some are just stylistic preferences of mine, some are doc standards. Take what you like from the optional suggestions, and make sure the universal updates are done (bold app names, remove center form images, etc).
This is a LOT of info, and I did not create the Meta acocunt to check, so take my suggestions with a grain of salt, since I am not following along on the facebook side =). Please let me know ifyou have any quesitons, and tag me again for another review when done. Thanks!!
56c86d7 to
7adecc1
Compare
|
@larm-odoo i've implemented your feedback! can you re-review please? |
3b55bc6 to
e85c5be
Compare
larm-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.
Great updates! I did see a few things that I recommend you double-check. First are the external links. I noticed some appear with the "http:://...." and some are just text, like "Meta developer console". I honestly think the second is better - just seeing the text, and not the URL itself. This can be done by rephrasing anyplace that just shows the URL. So instead of saying "Navitage to the Facebook Meta blah blah blah by going ot (web address). Instead, you can say " "Navitae to the "Facebook Meta blah blah blah" then... and have the part in quotes be the link. I think it's a cleaner way of viewing the information. This woudl require rewriting a lot of the links so it makes sense that the words are the link. Make sense?
Also, lots of th eimages have notations, which we no longer use. If you can retake the images and not add the red call-outs, that would be ideal. Some images I feel are not needed- example. the error images, the text int he error is in the written aprt, so they are redundant (except the first one, I woudl add the missing warning tet to the actual document so you can remove the image). Also, the 'add users to a chat' part - I feel that image is not needed- especialy when you remove the red markups, it'll seem confusing. I think you can eliminate the image and just include 'in the upper-right corner' to navigate them to where the button/icon is.
The image for the template placeholder example - I think that image can be removed, and you can add more text to the example. You cna include the palceholder text (what the user sees) then have the text appear as the receiver would. Make sense? This woudl allow for the elimination of the image.
Basically, if they are very big and uncrooppoed, and ponting out osmethig fairly obvious that you cna describe, I'd remove it. TImages hsoudl help clarify when necessary, but we don't need everything added as a visua;l.
We are almost there! Looking VERY good - and I know configuration instructions outside of Odoo is a pain, I've done it as well, so I appreciate all the work for this!
|
One more note - this PR title should change to a [IMP] not [ADD] because Add is only used when we are adding a doc that did not exist before, anything we improve or change for existing docs is an [IMP], regardless of how little or how much we change. Unless it's a tiny fix, like ONE typo- that would be a [FIX] PR. |
26b4bbc to
dc7e4d4
Compare
e879269 to
5aef795
Compare
35c67a0 to
4e13777
Compare
4e13777 to
217b0e8
Compare
|
@Felicious this is ready for review, PR and commit have been updated from |
Felicious
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.
What an outstanding first PR, @huisit !
This was a highly technical and challenging piece of content, and the scope alone made it non-trivial. The improvements you’ve made to language, clarity, and structure are substantial. The document is now easy to navigate, easy to search, and straightforward to follow 😄 Seeing the end result, it’s clear why this took time to get right -- the outcome justifies the effort. This is strong work!!
I really liked how you grouped the Quickstart section in particular. It's a term that a lot of companies in our industry uses, so it's easy to focus on the steps and allow users to orient themselves quickly. It's a prime example of how important naming and titling is in your craft of writing!
More broadly, you did a good job filling in missing context and reshaping content that was previously written primarily for highly technical readers. The document now works for less technical users without oversimplifying the material. Even where the overall structure remained similar, the document reads very differently due to clearer language, better prioritization, and thoughtful use of admonition blocks to abstract detail where appropriate. Moving away from raw link displays and consistently using descriptive link text also makes the document feel much cleaner and more intentional.
Overall, this document aligns extremely well with our style guidelines. It’s rare to see a first major doc come in this polished and this well-aligned. The amount of care and iteration you put into this shows 😊 And I barely had to do anything for my review!
After you look over some really minor changes I pointed out, this is ready for merge!
@robodoo delegate+
| :guilabel:`WhatsApp` in the left-hand side menu, click :guilabel:`API Setup`. Go to *Step 5: Add a | ||
| phone number*, and click :guilabel:`Add phone number`. |
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.
|
@robodoo fw=no |
|
Disabled forward-porting. |
… style updates Co-authored-by: larm-odoo <121518652+larm-odoo@users.noreply.github.com> Co-authored-by: Felicia Kuan <feku@odoo.com>
b834686 to
90eae4f
Compare
|
@robodoo r+ |
… style updates closes #15451 Signed-off-by: Rex Hu (rexhu) <rexhu@odoo.com> Co-authored-by: larm-odoo <121518652+larm-odoo@users.noreply.github.com> Co-authored-by: Felicia Kuan <feku@odoo.com>



documentation task card: https://www.odoo.com/odoo/action-4043/5257364
key changes @ line 181 / 192:
other:
This 17.0 PR can be FWP up to master. In the event of a merge conflict, tense changes can be left out for future work.