Skip to content

Conversation

@MattBred
Copy link

…e which product reference field to use

@MattBred MattBred changed the title Issue #2887930 by mbreden: Add logic and UX to let the customer choos… WIP: Issue #2887930 by mbreden: Add logic and UX to let the customer choos… Jan 10, 2018
*
* @return bool
* TRUE if the node has a product reference field.
* FALSE if the node does not have a product reference field.

Choose a reason for hiding this comment

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

I think it's safe to assume that people know a boolean is true or false and this could just say it's true if there's a reference field.

This could also say "node type" since it's checking the bundle, not an actual node.

'#title' => t('Display Type'),
'#type' => 'select',
'#description' => t('Select the display (node) type to use in import.'),
'#description' => t('Select the display (node) type to use in import. (The node must have a product reference field to show up in this list.)'),

Choose a reason for hiding this comment

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

You don't need parentheses here. Just say "The node must have..." or "Only nodes with product references are listed".

return array(
'#title' => t('Product Reference Field'),
'#type' => 'select',
'#description' => t('Select the product reference field to use in import.'),

Choose a reason for hiding this comment

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

"to use in import" isn't very clear about what happens with it unless you know how the import works under the hood. Can we say that it's the field that has the product variations?

// Only grab node types that have a reference to a product.
$node_types = array_filter(node_type_get_types(), function ($node_type) {
return commerce_xls_import_product_reference_field_exists($node_type->type);
});

Choose a reason for hiding this comment

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

This is the same code as in the import_start function. Should there be a function for it instead?

Copy link
Author

Choose a reason for hiding this comment

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

Drupal standards question: If there's a function that only an admin form(s) will use, where should it go? In this example, when the commerce_xls_import.export.inc file is running, the commerce_xls_import.admin.inc file isn't included, so we can't put the function there without doing an ugly drupal include thing.

@MattBred MattBred changed the title WIP: Issue #2887930 by mbreden: Add logic and UX to let the customer choos… Issue #2887930 by mbreden: Add logic and UX to let the customer choos… Jan 11, 2018
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.

2 participants