-
Notifications
You must be signed in to change notification settings - Fork 10.6k
[IMP] outlook mail_plugin: add solution for cookie issue #1645
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
|
@Feyensv could you review it please ? :) It's been a loooong while that I didn't made a PR for the documentation, let me know if I need to ping someone in particular for that |
|
There are codeowners for the doc as well now, so C3POdoo should assign them automatically, and after the codeowners have reviewed the PR, they ping us (doc-review team, ANV & I) for the tech review & r+. |
|
There was an issue with the code owners mapping. It's now fixed 🚀 |
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.
@lse-odoo Hello! Thanks for your contribution here. I'm requesting some small changes, mostly to improve wording and sentence flow, before we approve. Please let me know if you have any questions :)
| trial. This feature requires :ref:`prepaid credits <mail_plugins/pricing>`. | ||
|
|
||
| .. warning:: | ||
| If, after a short while, the panel is still empty. It is possible that your browser cookies |
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.
I made small changes to this sentence to fix a sentence fragment and improve flow.
If, after a short while, the panel is still empty, it is possible that your browser cookie
|
|
||
| .. warning:: | ||
| If, after a short while, the panel is still empty. It is possible that your browser cookies | ||
| settings policy did prevent it to load. |
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.
Small changes to improve sentence flow.
settings prevented it from loading.
| .. warning:: | ||
| If, after a short while, the panel is still empty. It is possible that your browser cookies | ||
| settings policy did prevent it to load. | ||
| Note that this policy does also change if you are connected in "Incognito" mode on your |
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.
Small changes to improve sentence flow.
Note that these settings also change if you are in "Incognito" mode on your
|
|
||
| To fix this issue, you can configure your browser to always allow cookies on Odoo's plugin page. | ||
|
|
||
| For Chrome in particular you can do so by following the guide at: |
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.
Small changes to improve wording and punctuation.
For Google Chrome, you can do so by following the guide at
| Note that this policy does also change if you are connected in "Incognito" mode on your | ||
| browser. | ||
|
|
||
| To fix this issue, you can configure your browser to always allow cookies on Odoo's plugin page. |
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.
Deleted "you can" so that it reads more like instructions, rather than just an option.
To fix this issue, configure your browser to always allow cookies on Odoo's plugin page.
|
|
||
| For Chrome in particular you can do so by following the guide at: | ||
| `https://support.google.com/chrome/answer/95647 <https://support.google.com/chrome/answer/95647#:~:text=Allow%20or%20block%20cookies%20for%20a%20specific%20site>`_ | ||
| then add `download.odoo.com` in the list of `Sites that can always use cookies`. |
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.
Small changes to keep the grammar consistent throughout the sentence.
and adding download.odoo.com to the list of Sites that can always use cookies.
Please note that I removed the backtick punctuation around "download.odoo.com" and "Sites that can always use cookies" in my corrected quote above, because Github reads it as formatting. You'll want to keep those backticks in the actual version.
| :align: center | ||
| :alt: Chrome cookie configuration to fix the empty panel issue | ||
|
|
||
| Once done, the panel needs to be open again. |
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.
Fixed a small grammar mistake.
Once done, the panel needs to be opened again.
Also, which panel are you referring to here? It might be good to specify.
41be2df to
7da13e7
Compare
|
Sorry, I didn't know about this PR and had to retrieve its information, you may have to re-approve it. |
|
@meng-odoo thank you for your review. I did update the text accordingly (in theory). Let me know your thoughts on the matter |
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.
@lse-odoo Hi! Thank you very much for making the suggested changes. All of the changes look good.
Upon a second review, I believe we should change a couple more key things before approving: making it a Tip instead of a Warning, and removing the image. Please see my comments below and let me know if you have any questions :)
| Only a limited amount of *Company Insights* (*Lead Enrichment*) requests are available as a | ||
| trial. This feature requires :ref:`prepaid credits <mail_plugins/pricing>`. | ||
|
|
||
| .. warning:: |
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.
Let's change this to a Tip instead of a Warning, because it informs readers about a useful trick, rather than warning them to proceed with caution.
.. tip::
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.
@meng-odoo Speaking about tips, you can click on the "Add a suggestion button" or use ctrl-g to easily make suggestions.
For example:
| .. warning:: | |
| .. tip:: |
|
|
||
| .. image:: outlook/cookie-fix.png | ||
| :align: center | ||
| :alt: Chrome cookie configuration to fix the empty panel issue |
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.
Let's delete this image. For platforms outside of Odoo, we want to link to guides from those platforms, rather than walking through the instructions on Odoo's documentation. So what you did here by linking to Google's support page is perfect, and we don't need to include more Google Chrome content in addition to that.
Also, we try not to use too many images. Although your image is helpful, it is not completely necessary for the reader to understand the instructions.
7da13e7 to
3fa47eb
Compare
|
@meng-odoo I do think that adding pictures can help customers visualise what to do and if the settings did change with time. But I do understand that it can indeed be confusing for some. I did remove it as you asked (and changed to a tip). |
meng-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.
@lse-odoo Hi, thanks again for making the suggested changes! Everything looks great. I'll go ahead and move this to the next stage in the review process.
AntoineVDV
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.
@robodoo r+
| If, after a short while, the panel is still empty, it is possible that your browser cookie | ||
| settings prevented it from loading. | ||
| Note that these settings also change if you are in "Incognito" mode on your | ||
| browser. |
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.
Nitpicking for your future PRs: It's better not to return to a new line if you don't intend to start a new paragraph. Indeed, single line breaks have no effect on the rendered HTML so it can be confusing for the technical reader to see useless line breaks.
| To fix this issue, configure your browser to always allow cookies on Odoo's plugin page. | ||
|
|
||
| For Google Chrome, you can do so by following the guide at: | ||
| `https://support.google.com/chrome/answer/95647 <https://support.google.com/chrome/answer/95647#:~:text=Allow%20or%20block%20cookies%20for%20a%20specific%20site>`_ |
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.
Nitpicking for your future PRs: you can break the line anywhere inside a link label.
| `https://support.google.com/chrome/answer/95647 <https://support.google.com/chrome/answer/95647#:~:text=Allow%20or%20block%20cookies%20for%20a%20specific%20site>`_ | |
| `https://support.google.com/chrome/answer/95647 | |
| <https://support.google.com/chrome/answer/95647#:~:text=Allow%20or%20block%20cookies%20for%20a%20specific%20site>`_ |
No description provided.