News:

Support the VirtueMart project and become a member

Main Menu

Custom Field "Price: : Free" at bottom

Started by jaminv, December 30, 2011, 01:27:51 AM

Previous topic - Next topic

jaminv

I'm working on a custom field plugin.

At the very bottom of my custom field display, it says "Price: : Free".  It is part of $field->display, but I didn't put it there.  It is not inside of an HTML tag, so I can't "display: none" it with CSS.

Question 1:  Where does it come from and how do I remove it?  It doesn't make any sense in relation to my plugin, and I don't want it there, for anyone who uses my plugin.

Question 2:  Why is it there?  I can't think of a scenario in which it actually makes sense, and it definitely doesn't in the case of my plugin.  I just want it gone.

It really seems to go against the nature of customization, in that it's there and my plugin has no control over it.

Question 3:  Why does it break all the rules?  It's not inside of an HTML tag = bad.  If I turn off all price displays, it's still there.  It's behaving very badly, and really seems like a flawed bit of code.

I've determined that the language codes are COM_VIRTUEMART_CART_PRICE and COM_VIRTUEMART_CART_PRICE_FREE, but the second colon... I don't know where that comes.  Means there's probably a hard-coded colon in there somewhere... nevermind that the whole thing appears to be hard-coded.  There's no way I want to tell my users that they have to override COM_VIRTUEMART_CART_PRICE and COM_VIRTUEMART_CART_PRICE_FREE, and then they're still stuck with a floating colon.

Question 4: Can we get it out of the system?  Please?
 

jaminv

#1
administrator/components/com_virtuemart/models/customfields.php :

line 583:
$group->display .= '<input type="hidden" value="'.$productCustom->value.'" name="customPrice['.$row.']['.$group->virtuemart_custom_id.']" /> '.JText::_('COM_VIRTUEMART_CART_PRICE').': '.$price ;

line 593:
$group->display .= '<input type="text" value="'.JText::_($productCustom->custom_value).'" name="customPrice['.$row.']['.$group->virtuemart_custom_id.']['.$productCustom->value.']" /> '.JText::_('COM_VIRTUEMART_CART_PRICE').': '.$price ;


The colons are redudant, as they are also contained in COM_VIRTUEMART_CART_PRICE, and they are also considered "bad practice", for exactly the same reason.

My field is of type 'E' and is affected by line 583.  I understand why the hidden field is there (kind of), but the price should be part of the plugin and not mandatory.  It also shouldn't display if price displaying is turned of in the config.

For now I'm commenting out the line, but I hate having to do that.  There's also no way I can ask my plugin users to do the same.

---

I noticed this as well in the edit view for my custom field.  There's a field for custom price appended at the very end of the display.  It doesn't really make sense that it be there... at least not in the context of my application.  It's less of a big deal here though, because the end user doesn't see it.

--Jamin


jjk

Quote from: jaminv on December 30, 2011, 01:27:51 AM
...COM_VIRTUEMART_CART_PRICE_FREE

Look at your frontend language file en-GB.com_virtuemart.ini around line 71 and replace
COM_VIRTUEMART_CART_PRICE_FREE="free" with COM_VIRTUEMART_CART_PRICE_FREE="No additional charge" or whatever you like to see there.
...or place your version of that line into the .../language/overrides folder (so it doesn't become overwritten when you update VM2)

It's used for displaying the additional price of product variants, i.e. Shirt white +0.00$, Shirt red +5.00$, etc.
Non-English Shops: Are your language files up to date?
http://virtuemart.net/community/translations

jaminv

#3
Sure, that makes sense... except for in the case of a plugin.

Take for example stockable.php (the plugin I am extending).  It actually selects a different sku and uses JS to update the price displayed above.  Displaying a variant price after that doesn't make any sense.  There's not 1 variant price for the plugin.  There are many.  Further, because the price isn't inside of an html tag, you can't use a script to change that price.  It's just static.  Further again, if you turn off price displays, it shouldn't show that price.

"Free", "No additional price", anything... it doesn't matter.  It doesn't make sense in the context of my plugin.  I don't want it there at all, but I can't get rid of it.

My proposals:

  • Remove the hard-coded colons
  • Put the price inside of a html tag with an id
  • Check to see if variant price display is enabled in the config

That's the minimum, I think, so that it's actually following the rules.  Right now it's just bad code.

Additionally, I'd really like to see an option where it checks to see if the custom field is actually a plugin, and omits the field or allows the plugin to choose whether or not it wants that displayed.  I'd be reasonably happy, however, if it just did the above three things. 

I don't think those three changes could possibly harm anything.

Studio 42

I have write the original stockable plugin and do some change on it.
First we have add a new trigger, this can help to have dynamic price from child or do some other stuff(you must update by the SVN to have the files)
I get the child id and check if the child is vallid(stock control...)

It can be better to wait a little before continue your code ...

jaminv

#5
The stockable plugin is great, but far too rigid.  I don't want a drop-down list, so I've expanded to allow other types of displays.  Specifically, I want a table with "add to cart" buttons for each child.  I've themed my pages so the default add to cart buttons are no longer there.  I know others want radio buttons, so I'm trying to facilitate that as well.  There will be other display options as well.

The table I'm displaying shows the price inside the table for each child.  It doesn't make any sense for there to be a price at the bottom.  The prices are already exactly where I want them.  But I can't get rid of it without hacking the core.

Your plugin has the same problem.  No matter what, the code I listed is coded into customfields.php and will display at the bottom of your plugin.  It doesn't make any sense with your plugin, either.  It's a flaw in the logic of that code.  It assumes that its necessary in all contexts, when in fact it is not.

---

No one seems to be getting the point of what I'm trying to say.  Let start with the double colon.  There's a colon hard-coded into the file, and a colon in COM_VIRTUEMART_CART_PRICE.  This is bad.  It looks really bad.  You can't remove the colon from COM_VIRTUEMART_CART_PRICE, because there are other places that do follow the rules and don't have a hard-coded colon.

Second, it displays a price even if all pricing options are turned off.  This directly goes against what the administrator wants.

Third, it's not inside of an HTML element.  That's just bad HTML, and it prohibits the administrator, developer, or template designer from changing it in any way.  It's just tacked on to the end.  What if a template designer wanted to move it or style it?  They can't apply a style unless it's applied to the entire containing element, which also contains the entire plugin.

Does no one else see this?

Milbo

of course we see it
hardcoded colons should be removed, no question. The new triggers also work mainly with pointers, so that you can manipulate really a lot. The plugin you are looking at is not adjusted to the new triggers.

and best is to speak with us in code, just post the problematic trigger and your suggested correction.
Should I fix your bug, please support the VirtueMart project and become a member
______________________________________
Extensions approved by the core team: http://extensions.virtuemart.net/

jaminv

#7
Thank you for listening.

Quote from: Milbo on December 31, 2011, 13:56:02 PM
and best is to speak with us in code, just post the problematic trigger and your suggested correction.

I did exactly that, but across two different posts.  Allow me to summarize:


in administrator/components/com_virtuemart/models/customfields.php :

line 583:
$group->display .= '<input type="hidden" value="'.$productCustom->value.'" name="customPrice['.$row.']['.$group->virtuemart_custom_id.']" /> '.JText::_('COM_VIRTUEMART_CART_PRICE').': '.$price ;


line 593:
$group->display .= '<input type="text" value="'.JText::_($productCustom->custom_value).'" name="customPrice['.$row.']['.$group->virtuemart_custom_id.']['.$productCustom->value.']" /> '.JText::_('COM_VIRTUEMART_CART_PRICE').': '.$price ;

Suggestions:

  • Remove the hard-coded colons
  • Put the price inside of a html tag with an id
  • Check to see if variant price display is enabled in the config

Here]s some corrected code:

$group->display .= '<input type="hidden" value="'.$productCustom->value.'" name="customPrice['.$row.']['.$group->virtuemart_custom_id.']" />';
if(!empty($currency->_priceConfig['variantModification'][0]) {
  $group->display .= '<div class="Priceplugin">'.JText::_('COM_VIRTUEMART_CART_PRICE').'<span class="Priceplugin">'.$price.'</span><div>';
}


Or something very similar to that.

---

Is there any documentation or examples of this new plugin methodology?

Thanks again,

--Jamin

jaminv

So I upgraded to 2.0.1N and this issue came back.  Apparently I had to hack the file to get rid of this (I hate having to do that).

It still displays this despite all price displays being turned off.
It still has a hard-coded colon despite their being a colon in the language file.
It would still be very nice if this entry were inside of a DIV or SPAN tag, so that it can be styled.

Is there any chance we can make these changes so that I don't have to hack the core again?  I'd be happy to jump on the SVN and do it, but I don't believe I have access.

Speaking directly in code, I'm talking about replacing line 602 of administrator/components/com_virtuemart/models/customfields.php with the following lines:


$group->display .= '<input type="hidden" value="'.$productCustom->value.'" name="customPrice['.$row.']['.$group->virtuemart_custom_id.']" />';
if(!empty($currency->_priceConfig['variantModification'][0])) {
     $group->display .= '<div class="price-plugin">' . JText::_('COM_VIRTUEMART_CART_PRICE') . '<span class="price-plugin">' . $price . '</span></div>';
}


Any help would be greatly appreciated.

--Jamin

jjk

#9
You may try the latest version 2.0.2 from here: http://dev.virtuemart.net/projects/virtuemart/files
I think there was a discussion about how to solve this problem yesterday.
Non-English Shops: Are your language files up to date?
http://virtuemart.net/community/translations

jaminv

This change has not been added to the latest version.  However, I am impressed with the number of changes that have been done.

Can we please be sure not to neglect this change?

THanks,

--Jamin

Milbo

Please look at this file, when it works correct for you and us, I committ it.

[attachment cleanup by admin]
Should I fix your bug, please support the VirtueMart project and become a member
______________________________________
Extensions approved by the core team: http://extensions.virtuemart.net/

jaminv

Thanks.  This works great for me.  Thank you very much for removing the colon and adding some tags, and also for allowing it to be turned off altogether.

Thanks again,

--Jamin

Milbo

Should I fix your bug, please support the VirtueMart project and become a member
______________________________________
Extensions approved by the core team: http://extensions.virtuemart.net/

jeffcdo

I'm using 2.0.2 but I'm still not clear on how to turn of the double colon.  My issue is using the custom product specification plug-in, I end up with something like this:

"Product Specification Price: : $2.00"

Ideally I'd like to get rid of the word "Price" and the double colon (or at least one of the colons).