Skip to content
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

Create Promotion Condition page. #264

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

Create Promotion Condition page. #264

wants to merge 10 commits into from

Conversation

nilswloewen
Copy link

This is a thread to get a page created on howto create a condition.

![Screenshot of add Promotion UI](promotion-example.png)


Minimum requirements to build a condition:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This begs the question "what are all the requirements?" You also have a colon in this title but not any others.

What do you think about calling this section "Creating Conditions"?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed on "Creating Conditions" but I think it should be "Creating Promotion Conditions"

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This page is in the "core" section, not the promotion section of the docs.

order meets a certain criteria and to return a boolean TRUE if so.

```
// *** Remember annotations are inherited from ConditionInterface.***
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does this mean? Annotations aren't inherited.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think Travis means annotations aren't inheritable.

public function evaluate(EntityInterface $entity) {

// This line is essential as it ensures this condition will only run for the
// 'entity_type' declared in this condition's annotations.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't mean it only runs for that entity type, it ensures the site will crash if it's not that type (assuming nothing catches the exception).

@@ -4,4 +4,7 @@ taxonomy:
category: docs
---

See [Core Components / Conditions](https://docs.drupalcommerce.org/commerce2/developer-guide/core/conditions)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should be able to link to pages internally rather than by the whole domain and everything.

* If the total quantity of all order items is greater than or equal to the
* quantity that the user set in the configuration form, return TRUE. If not,
* return FALSE and the promotion will not be applied to the order.
*/
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As above I'd leave out most of all of these explanations for the obvious stuff - the Calculator:add bit is self-explanatory and this following bit it just knowing what the comparison operators do.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, this comment style is not Drupal code style friendly.

![Screenshot of add Promotion UI](promotion-example.png)


Minimum requirements to build a condition:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed on "Creating Conditions" but I think it should be "Creating Promotion Conditions"

order meets a certain criteria and to return a boolean TRUE if so.

```
// *** Remember annotations are inherited from ConditionInterface.***
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think Travis means annotations aren't inheritable.

$quantity = '0';
foreach ($order->getItems() as $order_item) {

// First assess if each order item can apply to this condition.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

code formatting, needs spaces here.

}
}

// Get the quantity from each item and add to a running total.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a fan of this indentation thing. If you want to have inline comments, break the code up into smaller blocks of code with your comment in between.

* If the total quantity of all order items is greater than or equal to the
* quantity that the user set in the configuration form, return TRUE. If not,
* return FALSE and the promotion will not be applied to the order.
*/
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, this comment style is not Drupal code style friendly.

@@ -4,4 +4,7 @@ taxonomy:
category: docs
---

See [Core Components / Conditions](https://docs.drupalcommerce.org/commerce2/developer-guide/core/conditions)
for an explanation of a promotion condition.

! We need help filling out this section! Feel free to follow the *edit this page* link and contribute.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You'll want to remove the ! We need help... section.

Copy link
Collaborator

@lisastreeter lisastreeter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I apologize for not looking at this PR more carefully until now. When I saw the, "Create Promotion Condition" page title, I assumed this was work for the "Create a condition" page within the "Promotions" section of the documentation. In the meantime, a general "Core" condition page was merged in. There's a lot of good information here, though. I'm wondering whether it would make sense to rework the start of this a bit, to make it specific to "Promotion" conditions, since the rest of the document focuses on that. And then it would be a great addition to the "Promotions" section of the documentation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants