commerce_product_reference.module uses wrapping HTML to assign a contextual class to product fields with multiple values, for use in automatically updating these fields' values when product attributes are chosen (i.e. changing an image when a color option is chosen, etc.)

Currently, a <span> is used, which can cause a few minor problems when styling Products (particularly, extra space is added below the field because of the span's line height). The single class name used node-[ID]-product-[field_name], while necessary for the ajax functionality, is far too specific for styling purposes.

The attached patch changes this wrapper to a <div> and adds two generic classes, product-field and [field_name].

Everything seems to be working fine with this change in my localhost.

Thanks!
Sheena

CommentFileSizeAuthor
#7 commerce.product_reference_classes_1280894_07.patch1.34 KBrfay
better-formatting-product-reference-field-wrappers.patch1.23 KBsheena_d

Comments

fakingfantastic’s picture

fakingfantastic commented

I +1 this patch, it does the job, but there is still a bunch of HTML just to spit out an image. In one case, i got this...

<div class="node-8-product-field-image product-field field-image">
  <div class="field field-name-field-image field-type-image field-label-hidden">
     <div class="field-items">
        <div class="field-item even">
           <img typeof="foaf:Image" src="/sites/default/files/my_file.jpg" alt="">
        </div>
      </div>
    </div>
</div>

I understand the bloat is the Field API, so there's not much to do there. But what about moving node-8.... to an ID ... since it's an ID :)

What are we all thinking?

sheena_d’s picture

sheena_d commented

I think moving node-8.... to an ID will require changes in the ajax functionality and it could also cause multiple IDs on a page if you happen to have a view of node teasers on the full node page. Although, I guess multiple IDs is a threat anytime you have node teasers on a full-node page.

rszrama’s picture

rszrama commented
Issue tags: +1.1 blocker

Tagging.

Jeff Burnz’s picture

Jeff Burnz commented

Couple of thoughts:

"product-field" - seems too generic to be useful.

"field-image" - should be name spaced, i.e. "product-field-image"

I'll test the patch out shortly.

rszrama’s picture

rszrama commented

Yeah, we can probably make that commerce-product-field to be more helpful. field-image is just the name of the field classified, so we can go with the Drupal syntax (field-name-field-image) if it's worth having in there twice. We could always try to combine it somehow w/ commerce-product-field, but that'd make for some ungainly classes. : P

Jeff Burnz’s picture

Jeff Burnz commented

Well, the reason I would want those classes name spaced is so they are always unique and never duplicate what theme_field is giving us already - so better to have ungainly long classes that are reliable and usable. Duplicating theme_fields classes could spell bad news depending on the themes CSS.

rfay’s picture

rfay
he,him
Primary language English
Location Palisade, CO, USA
commented
StatusFileSize
new commerce.product_reference_classes_1280894_07.patch1.34 KB

This looks fine to me. Here's a slightly revised patch that tries to address the above comments.

I've tested this with the standard multiple-tshirt-attribute test and Bartik. Seemed fine.

rszrama’s picture

rszrama commented
Status: Needs review » Fixed

I had to duplicate the new classes in the AJAX refresh callback used for the Add to Cart form. I also switched product-field to commerce-product-field in line with what I think Jeff's comment #8 was indicating. Everything tests out fine for me, too.

Commit: http://drupalcode.org/project/commerce.git/commitdiff/3c58c5c

rszrama’s picture

rszrama commented

Bah, in my duplication of the new classes into the Cart module I accidentally removed the assignation of the replacement class to a standalone variable for use in the AJAX command. Fixed now.

Commit: http://drupalcode.org/project/commerce.git/commitdiff/95843b2

rszrama’s picture

rszrama commented
Category: bug » task

Automatically closed -- issue fixed for 2 weeks with no activity.