Product 'price' index formatted too early #3

New issue

Closed

opened 2020-04-19 16:18:53 -05:00 by gregzawadzki · 4 comments

gregzawadzki commented

2020-04-19 16:18:53 -05:00

(Migrated from github.com)

github.com/thirtybees/blockbestsellers@3b3303af00/blockbestsellers.php (L388)

I don't think it should be this way. It was since always that it was raw floats assigned to templates. It cannot be used for microdata in that way anymore and for any ifs and calculations.

https://github.com/thirtybees/blockbestsellers/blob/3b3303af0050415e749f7bb69312abee1479dbb3/blockbestsellers.php#L388 I don't think it should be this way. It was since always that it was raw floats assigned to templates. It cannot be used for microdata in that way anymore and for any ifs and calculations.

getdatakick commented

2021-03-02 08:13:57 -06:00

(Migrated from github.com)

For compatibility reasons we need to keep this as it is. We could expose another field, for example $row['price_raw']

For compatibility reasons we need to keep this as it is. We could expose another field, for example `$row['price_raw']`

gregzawadzki commented

2021-03-03 06:49:57 -06:00

(Migrated from github.com)

From what I understood, TB puts some pressure on template re-usage (one template used in many places).
From one side I understand you, from other side, not each template could be used by blockbestsellers then.
Making it compatible with rest things could acually allow to {include} other template here and would make it more consistent.
Just saying, you will do whatever you want.

From what I understood, TB puts some pressure on template re-usage (one template used in many places). From one side I understand you, from other side, not each template could be used by blockbestsellers then. Making it compatible with rest things could acually allow to `{include}` other template here and would make it more consistent. Just saying, you will do whatever you want.

getdatakick commented

2021-03-03 07:12:49 -06:00

(Migrated from github.com)

Templates can, and often are, overwritten by themes. It makes sense, because theme wants to integrate the functionality seamlessly with the rest of the design and ui components.

Therefore, it's not a good idea to change the smarty variables module expose to the view layer, or their semantics. That is just asking for problems.

Of course, we could (and should) change and this particular template in community and niara themes. But that doesn't change the fact that similar override can be used in some third party theme, such as panda or warehouse.

Templates can, and often are, overwritten by themes. It makes sense, because theme wants to integrate the functionality seamlessly with the rest of the design and ui components. Therefore, it's not a good idea to change the smarty variables module expose to the view layer, or their semantics. That is just asking for problems. Of course, we could (and should) change and this particular template in community and niara themes. But that doesn't change the fact that similar override can be used in some third party theme, such as panda or warehouse.

gregzawadzki commented

2021-03-03 07:30:43 -06:00

(Migrated from github.com)

I understand this change would be problematic and it's a lesser evil here. All depends what is less evil here. True, that TB is not really in position (hopefully yet) to dictate such changes, and you have to take Panda and Warehouse into consideration. But maybe with version 2.0 ;)
There will be some day when compatibility has to be broken, or you won't move from where you are. So you can store it and add a "break compatibility someday feature" label 😆

I understand this change would be problematic and it's a lesser evil here. All depends what is less evil here. True, that TB is not really in position (hopefully yet) to dictate such changes, and you have to take Panda and Warehouse into consideration. But maybe with version 2.0 ;) There will be some day when compatibility has to be broken, or you won't move from where you are. So you can store it and add a "break compatibility someday feature" label :laughing: