-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Minor updates in promotions overview documentation #3121
Minor updates in promotions overview documentation #3121
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.
I left a comment/question but it's not blocking. Thanks!
@@ -140,7 +138,7 @@ See the `eligible?` method defined in the [Spree::Promotion | |||
model][spree-promotion]: | |||
|
|||
```ruby | |||
# models/spree/promotion.rb : line 123 | |||
# models/spree/promotion.rb : line 120 |
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'm not sure it's a good idea to keep this line number. It's very hard to maintain and it's super easy to find a method into a file with all editors or browsers. Not sure if it's better to take advantage of this change to remove it or we could remove it also in other places (if any) with another PR. WDYT?
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.
Updated Thanks for the suggestion 😄
The promotions documentation `def eligible?` method line number is unnecessary this updates the documentation reference removing the line 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.
Looks good to me. Thanks @JuanCrg90! 🏅
Re-running build to get ✅ |
Description
This pull request adds some minor updates in the promotions documentation.
eligible?
method line numberChecklist: