7455/price slider #52
Conversation
# Conflicts: # main/src/view/frontend/requirejs-config.js # main/src/view/frontend/templates/layer/filter.phtml
- add custom handle markup to slider - fine-tune handle position to fit within container boundaries - add standard button style
…to2 into 7455/price-slider
They are retrieved from the Solr "stats" function which automatically displays the min and max values for selected fields.
The step size depends on minimum and maximum available values. It is calculated in a way that there are always between 10 and 100 steps in between, and it's a number like 0.1, 1, 10, 100, ...
It is always active for decimal attributes as we don't support the original implementation of decimal filters.
AdditionalParameter `AttributeRepository $attributeRepository` needs to be created as mock.
|
Could you review now @schmengler? |
| * @param float|string $toValue | ||
| * @return float|\Magento\Framework\Phrase | ||
| */ | ||
| protected function _renderRangeLabel(Subject $subject, $fromValue, $toValue) |
There was a problem hiding this comment.
with current Magento 2 standards it should be private and without underscore
| if ($this->isPriceFilter() && !$this->scopeConfig->isSetFlag('integernet_solr/results/use_price_slider')) { | ||
| return false; | ||
| } | ||
| return $this->isPriceFilter() || $this->isDecimalFilter(); |
There was a problem hiding this comment.
can the slider be used for other decimal attributes? then the name "price slider" is misleading
| */ | ||
| public function getPriceFilterUrlWithPlaceholder() | ||
| { | ||
| $query = [$this->getFilter()->getRequestVar() => ['priceRange']]; |
There was a problem hiding this comment.
I'd suggest to extract 'priceRange to a PRICE_RANGE_PLACEHOLDER constant, to not have a magic string in two different methods
| */ | ||
| private function __construct(SolrResponse $solrResponse) | ||
| private function __construct(SolrResponse $solrResponse, AttributeRepository $attributeRepository) |
There was a problem hiding this comment.
Until now this was a domain class that only mapped one format into another. Pulling in repositories seems a wrong dependency to me.
How could we avoid this? I'm not sure yet, maybe a parameter array $attributesWithInterval = ['price'] and move loading these attribute codes to the client (SolrAdapter)?
| </div> | ||
| <p> | ||
| <a class="action primary" | ||
| href="<?php echo $block->escapeUrl($filterItem->getPriceFilterUrlWithCurrentValues()) ?>" |
There was a problem hiding this comment.
Should be OK in this case, just FYI:
I've previously done some investigations about the escape methods of Magento and escapeUrl is in most cases not the best solution for URLs (in Magento 2.0 it even was utterly useless)
What it does:
- try to prevent XSS by removing
javascript:,vbscript:anddata: htmlspecialchars- apply
escapeHtml(), i.e.htmspecialcharsagain (looked that up right now, that seems to be new and could lead to new problems)
If we want to escape internal URLs, generated by Magento, the best method is escapeHtmlAttr() - because it's in the href attribute and we should have proper escaping for that context (in contrast to escapeHtml() which is for inner text in the HTML elements).
There is no risk of XSS, we just don't want to break the HTML
| <input class="tickbox__field" type="checkbox" > | ||
| </label> | ||
| <?php /* @escapeNotVerified */ echo $filterItem->getLabel() ?> | ||
| <script> |
There was a problem hiding this comment.
I'm not sure if we're supposed to use inline require() like this. The Magento way would probably be to define the component in plain JS and attach it to the HTML elements (valueContainer+filterLink or rather a parent of both) via data-mage-init - parameters like "url", "unit", "min" and "max" then would be passed there
| <?php if ($this->helper('\Magento\Catalog\Helper\Data')->shouldDisplayProductCountOnLayer()): ?> | ||
| <span class="count"><?php /* @escapeNotVerified */ | ||
| echo $filterItem->getCount() ?><span class="filter-count-label"> | ||
| <?php if ($filterItem->getCount() == 1): ?><?php /* @escapeNotVerified */ |
There was a problem hiding this comment.
@escapeNotVerified looks like copied from core code, but it wouldn't hurt to actually escape
Allow price slider and decimal attribute sliders in search results