Skip to content
This repository has been archived by the owner on Oct 18, 2023. It is now read-only.

Allow setting attribute weights per attribute set. #47

Conversation

nikunjkotecha
Copy link

No description provided.

@nikunjkotecha
Copy link
Author

nikunjkotecha commented Jul 10, 2018

cc @sdelbosc @vbouchet31

@nikunjkotecha nikunjkotecha force-pushed the attribute_weights_per_attribute_set branch from 627bcf6 to c7ac5b4 Compare July 10, 2018 17:36
@coveralls

This comment has been minimized.

@malachy-mcconnell
Copy link
Contributor

Hi Nikunj, there are some nice ode changes here but we expect attribute weights to by synchronized from the ecommerce system and not set in Drupal. Do you agree? Is this a quick-fix until we can implement synchronisation of attribute weights (sort order)?

@nikunjkotecha
Copy link
Author

Hi @malachy-mcconnell, I checked the code around automated synchronisation of attribute weights. But that still doesn't support multiple attribute (like the old code). I updated the code around extractConfigurableOptions too in the PR and updated logic should work as is. Probably we may want to set default value in config to attribute_weights: { } as not all systems would have size, otherwise everything here works.

Sortable weights is something we will need to define - reason behind that - we may not have all the attributes in taxonomy terms and it is a performance killer which we should avoid till really required in the project.

@nikunjkotecha nikunjkotecha force-pushed the attribute_weights_per_attribute_set branch from c7ac5b4 to 844ee31 Compare July 16, 2018 18:46
@malachy-mcconnell
Copy link
Contributor

On hold. Do not merge until the requested Dependency Injection can be added.

…s_per_attribute_set

Conflicts:
	modules/acm_sku/src/Plugin/AcquiaCommerce/SKUType/Configurable.php
@nikunjkotecha
Copy link
Author

fixed conflicts

@miromichalicka miromichalicka merged commit 5f8ed45 into acquia:8.x-1.x Mar 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants