-
Notifications
You must be signed in to change notification settings - Fork 10k
[IMP] CRM: lost opportunities update #12151
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
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 @justinmallette !
I can tell you took the time to go through our style guide carefully—great work! 😊 The improvements you made look solid, and the updated images are a great touch. The only reason I’m pushing this draft back is for the icon usage.
All my other suggestions are purely optional—just some rewording tweaks to make the doc even more concise and a few pointers to help you get more familiar with our team's guidelines on guilabel and menuselection tags. Once you've had a chance to review and implement any changes, tag me for a second review!
One small housekeeping note—under the Labels section of the PR, make sure to assign a point value of 2 so your work is properly counted in our reporting software.
You're doing amazing, and congrats on setting the record for the fastest first PR on the team! 🚀 Hope all that waiting to install the repo, Vale, and everything else was worth it. 😆
6fbd2e2
to
96e5d59
Compare
@Felicious Made all the requested changes. Ready for another 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.
Great work, @justinmallette !
The content looks solid—I just have a few small RST formatting tweaks. Once you've made those updates, tag me again, and I should be able to approve it after a quick ~5 min pass! 🎉
Also, this PR is worth 2 points! If the task said 1, I'm re-evaluating it to 2 points, because you've done a number of changes to update the content and accuracy to align with the 18.0 UI and out latest standards!
57b9258
to
fdf9b84
Compare
@Felicious Ready for another review |
Great job fixing the icon descriptors and backticks! We just need to find an alternative rewrapping extension for you 😊 (Oh and those darn kit changes are still there 😭 ) |
6ae66c8
to
95172f8
Compare
LMK if the text is wrapped! I think I got a newer version of the extension to work (and those kit changes are reverted :)) |
425439e
to
f1572cd
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.
Amazing job, @justinmallette !
Everything works as expected, and the instructions are accurate and align with the 18.0 UI! You've also made great improvements for brevity and readability—nice work! 🎉
After taking a look at my optional comments, feel free to tag @StraubCreative for final review!
da40b24
to
78d83f9
Compare
@StraubCreative Ready for your 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.
Pausing here after a few comments— a lot of these changes didn't need to be made as they'd fall under superficial elements of "craft" in our quality matrix.
In our guidelines we explicitly say that rewrites need to having compelling reasons behind them, which I'm not seeing here so far (correct me if I'm missing something!).
My suggestion is to revert back all of superficial changes (either manual c/p + force push or git reset --hard [commit_hash]
) and keep only the necessary ones that center around adding or updating context, fixing errors or inaccurate information, deepening product education, etc. (not writerly things like small word choices).
Framing the feedback into actionable learning:
- discover how to quickly revert changes in Git
- learn how to revert changes in batch across multiple PRs
- (re)familiarize yourself with the team guidelines around rewrites
- reflect on the cost of speed vs. quality, and calibrate accordingly for higher and more precise impact in submitted work...many PRs with the same issue (e.g. missing conjunctions, unnecessary rewrites, etc.) means time gets wasted in a compounding way due to unnecessary review(s) and discussion, duplicate or redundant work, having to redo or fix mistake, etc.
Hope this helps explain the logic of my change requests 🙏
Please let me know if you have any questions otherwise I can take another look after (click the 🔄 gear next to my u/n in Reviewers box).
choose an existing lost reason. If none of these are appropriate, create a new one by entering it | ||
into the :guilabel:`Lost Reason` field, then select :guilabel:`Create`. |
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.
Pretty sure this sentence and others are missing "and" conjunctions?
"If" statements ideally follow with a quick "then" consequence.
Another example of a sentence that did not need to be rewritten.
choose an existing lost reason. If none of these are appropriate, create a new one by entering it | |
into the :guilabel:`Lost Reason` field, then select :guilabel:`Create`. | |
choose an existing lost reason. If none of these are appropriate, then create a new one by entering it | |
into the :guilabel:`Lost Reason` field, and then clicking :guilabel:`Create`. |
d9fd056
to
b49b8f9
Compare
b49b8f9
to
c0e1c31
Compare
@StraubCreative I've taken another look through the file, and I believe its ready for another look from you. Please let me know if there's still something you're looking for but not seeing in this latest version |
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 @justinmallette
Approving with a few comments around UI terminology
Can pass to Sam when you've taken a look, thanks!
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.
28cdb7b
to
a899e28
Compare
@samueljlieber Ready for a tech 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.
Hey @justinmallette! Nice work on this improvement PR! Your changes look great to me. I am approving with two small fixes. Please address these before merge.
..
@robodoo delegate=justinmallette
fcae73b
to
efdf884
Compare
Co-authored-by: Sam Lieber (sali) <36018073+samueljlieber@users.noreply.github.com>
efdf884
to
50c2770
Compare
@robodoo r+ |
closes #12151 Signed-off-by: Justin Mallette (juma) <juma@odoo.com> Co-authored-by: Sam Lieber (sali) <36018073+samueljlieber@users.noreply.github.com>
closes #12151 Signed-off-by: Justin Mallette (juma) <juma@odoo.com> Co-authored-by: Sam Lieber (sali) <36018073+samueljlieber@users.noreply.github.com>
Updated images to new style standards. Fixed UI discrepancies.