-
Notifications
You must be signed in to change notification settings - Fork 10.4k
[IMP] surveys:update get started docs #2534
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
Hey There @mivu-odoo , New content and screenshots for the Surveys documentation. Can you please give this a review? Then I'll send it over to Zac. Thanks! |
854cbbd
to
ea62e34
Compare
Hi @ksc-odoo just letting you know I pushed a commit to fix some build errors that were keeping the PR from passing the ci/documentation code check! I'll update one again when I do content review! |
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.
Hello @ksc-odoo!
This will be part one of my final review; I figured I can publish these comments so you can get started on the changes while I continue reviewing the other RST files.
This review only contains comments on the create.rst
file. Please let me know if you have any questions about them. I will continue reviewing scoring.rst
and time_random.rst
in the meantime, and notify you when I finish making comments and publish them to this PR.
ea62e34
to
f814c14
Compare
f814c14
to
1874eed
Compare
Hi @ksc-odoo! I opened your feature branch up on my computer to see if I can tinker around with the build errors. Surprisingly, my terminal showed a different build error from the one you showed me earlier in the day. I'm wondering if the sphinx configuration on your computer might need to be updated, but that's a question for Zac when he comes back. I fixed the build errors that were showing on my terminal and pushed commit 1874eed, and the PR is showing that all code checks have passed now. After I make more comments on this PR, make sure to run |
1874eed
to
260b407
Compare
934e2b6
to
36b7fc2
Compare
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.
Hello @ksc-odoo!
I left some comments on scoring.rst
. There are slight differences in field names between Odoo 14 and Odoo 15. After this PR is merged and forward-ported, we can open another PR targeting the 15.0 branch.
I still have to review time_random.rst
, and I might take another pass at create.rst
. I will make another comment once I finish looking at those RST files. Feel free to start working on the changes to scoring.rst
in the meantime. 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.
Left some comments for time_random.rst
! Last thing for me to do is to take a second look at create.rst
, but feel free to work on the changes to time_random.rst
and scoring.rst
while I do that. Thank you!
content/applications/marketing/surveys/overview/time_random.rst
Outdated
Show resolved
Hide resolved
content/applications/marketing/surveys/overview/time_random.rst
Outdated
Show resolved
Hide resolved
content/applications/marketing/surveys/overview/time_random.rst
Outdated
Show resolved
Hide resolved
content/applications/marketing/surveys/overview/time_random.rst
Outdated
Show resolved
Hide resolved
content/applications/marketing/surveys/overview/time_random.rst
Outdated
Show resolved
Hide resolved
content/applications/marketing/surveys/overview/time_random.rst
Outdated
Show resolved
Hide resolved
content/applications/marketing/surveys/overview/time_random.rst
Outdated
Show resolved
Hide resolved
content/applications/marketing/surveys/overview/time_random.rst
Outdated
Show resolved
Hide resolved
36b7fc2
to
cc1af20
Compare
cc1af20
to
09b04e6
Compare
UpdatesAs of 09b04e6:
Notes@ksc-odoo the CI checks are not passing on this latest commit due to bountiful trailing whitespaces. You can see them here: https://runbot.odoo.com/runbot/build/21751024. The checks did not pass on prior commit cc1af20 because there were merge conflicts against Reminders
Next stepsBefore continuing content review with @mivu-odoo, please address all of the instructions in this comment and fix all of the CI errors until they pass. Reach out if you need help. Tip: this extension should help out with trailing whitespaces, as it removes them whenever you save the file. Thanks! |
09b04e6
to
8b82984
Compare
@ksc-odoo checks are not passing on 8b82984. The build errors are listed on the ci/documentation Details link at the bottom of the page. Try again and only request a review when the checks pass. Thank you. cc: @mivu-odoo |
Updates in b9f7593:
Updates in 9f06c36:
@mivu-odoo ready for another review 🙂 |
9f06c36
to
d9c55f5
Compare
Fixed commit history in d9c55f5. |
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 @ksc-odoo
This was a pleasure to read. I like how the instructions were really broken down and easy to understand. I also appreciated how each doc was laid out with bite-size paragraphs.
I have a number of change requests to tighten everything up. There are a few CRs that must be addressed due to things like typo/grammar or line breaks, but a majority of them are more or less for your consideration as doc owner.
If you agree with the content changes listed below, I can push them up and we can pass over to DR right away. Otherwise let me know what you'd like to keep or disregard, per comment.
Thanks and looking forward to your response 🙂
content/applications/marketing/surveys/overview/time_random.rst
Outdated
Show resolved
Hide resolved
content/applications/marketing/surveys/overview/time_random.rst
Outdated
Show resolved
Hide resolved
content/applications/marketing/surveys/overview/time_random.rst
Outdated
Show resolved
Hide resolved
content/applications/marketing/surveys/overview/time_random.rst
Outdated
Show resolved
Hide resolved
content/applications/marketing/surveys/overview/time_random.rst
Outdated
Show resolved
Hide resolved
@StraubCreative Feel free to make any changes you'd like to tighten things up. Do whatever you feel is best. Thanks |
d9c55f5
to
5c159be
Compare
Latest round of CRs addressed on 5c159be. These docs on Surveys are ready for your review @odoo/doc-review 😉 |
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.
Feel free to merge after the comments have been resolved (approved or rejected).
@robodoo delegate+
content/applications/marketing/surveys/overview/time_random.rst
Outdated
Show resolved
Hide resolved
content/applications/marketing/surveys/overview/time_random.rst
Outdated
Show resolved
Hide resolved
5c159be
to
ceab2de
Compare
ceab2de
to
f2a8375
Compare
I'm sorry, @StraubCreative. I'm afraid I can't do that. |
@AntoineVDV robodoo being grumpy 😭 |
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.
delegate+
delegates the review rights to the author of the PR; delegate=<someone>
delegates the rights to <someone>
.
@robodoo r+
closes #2534 Signed-off-by: Antoine Vandevenne (anv) <anv@odoo.com>
Task ID: 2938139