Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
It would be really handy if we can use page_manager + view (display block) + facets. That gives a lot of flexibility for the page layout.
Comment | File | Size | Author |
---|---|---|---|
#49 | views_block_display-2678486-49.patch | 16.56 KB | borisson_ |
Comments
Comment #2
Leksat CreditAttribution: Leksat at Amazee Labs commentedHere is a patch that extends the existing classes for that. Made on the base of 8.x-1.0-alpha.
Comment #5
Leksat CreditAttribution: Leksat at Amazee Labs commentedHere is the same patch, but it's more readable since file renames are handled.
Comment #6
Leksat CreditAttribution: Leksat at Amazee Labs commentedMost probably we'll move to the dev version soon, so I will create a patch for 8.x-1.x shortly after that.
Comment #7
borisson_I had already spent some time on a patch that's similar to this one in an older issue, but we closed that because there were more things going on in that issue.
#2598902: Add 'show only on a page that contains the facet source' setting and a second facetsource plugin to include view blocks
https://www.drupal.org/node/2598902#comment-10639420 was my latest patch, intead of merging both facet sources, what you've done here, I chose to add an extra facet source. I think the approach in this patch makes more sense as it leads to less duplicate code.
One thing I'm afraid about is the getPath and isRenderedInCurrentRequest methods. Not sure how those are going to work for blocks. We would love to get support for blocks, but this is not very easy, considering the fact that blocks can be rendered in isolation.
I'm looking forward to seeing a patch that applies to see how many of our current tests are going to break because of this.
Comment #8
borisson_I rerolled the patch so we can see what the bot thinks.
Comment #11
Leksat CreditAttribution: Leksat at Amazee Labs commented@borisson_, thanks for your reply and reroll!
Initially I also started with splitting up SearchApiViewsPage to SearchApiViews (as a base), SearchApiViewsPage, and SearchApiViewsBlock. But I met some issues. The biggest one is: there is an identifier that's used for two different terms:
- search id
- facet source id (I guess, facet plugin id is the same?)
For example, we have "search_api_view:my_view:my_display" id.
- as a search id it is built as
"search_api_view:" . $view_name . ":" . $display_name
- as a facet source id:
$facet_plugin_type . ":" . $view_name . ":" . $display_name
So, when I added plugin type "search_api_view_block", the source id became "search_api_view_block:my_view:my_display", and nothing working.
UPD: just checked your patch and found that you are aware of this :)
Comment #12
Leksat CreditAttribution: Leksat at Amazee Labs commentedJust found that my rollout is a bit different. It also updates \Drupal\facets\Plugin\facets\url_processor\QueryString::buildUrls().
Comment #13
Leksat CreditAttribution: Leksat at Amazee Labs commentedGreen light! :D
Comment #14
borisson_Awesome; looks like I made a mistake rebasing the patch. This looks great. All we need to do is add a view that has a block display to the tests and see if the integration tests also work with that block. I'll have a stab at that today.
Comment #15
borisson_So, this adds a basic integration test for the functionality. The rendered in current request functionality is not tested, because it's not expected to work yet.
Not sure if we should just mention this in the readme / code and be done with it or if we should do something else with that. I'll ping @nick_vh, he might remember more from that discussion.
Comment #18
Nick_vhOverall I agree with the approach, but it does lack some information as to why we initially only supported pages. It's also good that this is limited to blocks but it should make that more clear in the description but also in the UI of facets. I am sure if there is another view display that is just like a page but has a different name, people will get confused because they are not seeing that facet source in the list.
Let's be very exact in our description what this does. It represents a facet source which has support for search_api_views pages and blocks.
Also where we see the if, it should give a much better answer as to why we are only supporting those two. Why not other view modes. Do we ever want to make this configurable so that people can change this at their own risk, say integration modules that add support for other view displays?
This should be FALSE, if we are uncertain let's make sure we do not fool anyone and do not give it the benefit of the doubt. Also, it would be good to add an explanation why it is not possible to know if a block is embedded on a page.
Same here, add description that this only derives a facet source for search_api_views pages and blocks.
Is this really the "name" or is it the $display_id / $id?
Should we also add in the label whether it came from a block or a page? Eg add in the $display_info['display_plugin']?
Silly other remark, do we refer to "Search api" or "Search API" or "Search Api". Would be good to be consistent with what search_api calls itself.
is this going to create issues? There's probably a reason why the slash was added?
Comment #19
borisson_Fixed the test fail by making sure to use the correct facet source for testing. Also added extra comments in the
isRenderedInCurrentRequest
method. to fix #18.3I'll look at the other remarks now. Thanks for the awesome review.
Comment #22
Leksat CreditAttribution: Leksat at Amazee Labs commentedActually, I think that we can support all view display types:
- instead checking
$display_type == 'page'
, we can check\Drupal\views\Plugin\views\display\DisplayPluginInterface::hasPath()
- isRenderedInCurrentRequest() also can use display's hasPath() + getPath() to determine the visibility
One side effect could be that /admin/config/search/facets page will become huge. But we can hide sources having zero facets, right?
Comment #23
borisson_#18
Tests will still fail, I'll have another look at that now.
Comment #26
borisson_Test should be green now.
Comment #27
borisson_This is very interesting, but I think I'd prefer to be conservative here, if another module adds a new display type, that doesn't mean we can automatically support that. (e.g. data export)
Comment #30
borisson_And another test broke, this fixes that one. I also added an interdiff to #15.
Comment #31
Leksat CreditAttribution: Leksat at Amazee Labs commentedAbout slashes in URLs.
As far as I can see, Drupal 8 uses leading slash in URLs in all methods. Probably, views is the last module where this was not changed.
Comment #32
Nick_vhSo that means that change needs to be reverted in the patch or?
Comment #33
Leksat CreditAttribution: Leksat at Amazee Labs commentedI'd prefer if facets behaves the same as most of D8 core, so uses leading slashes in URLs. That's why I changed \Drupal\facets\Plugin\facets\facet_source\SearchApiViews::getPath() to return an already slash-prefixed path.
Comment #34
Leksat CreditAttribution: Leksat at Amazee Labs commentedFYI, created a core issue #2680209: [meta] Advanced search/view with advanced layout (draft).
Comment #35
borisson_This needed a reroll.
Comment #38
borisson_Looks like something I committed today broke the patch in #30. I can't seem to find why - yet.
Comment #39
borisson_Setting to NR for the bot.
Comment #42
borisson_Comment #43
borisson_Needed reroll.
Comment #44
borisson_I rolled a new patch after the last commits and created this patch with
git diff -M
to make the patch smaller. I'm pretty sure this is ready - if we can get someone to set this to RTBC we can commit this.Comment #47
borisson_Looks like I was wrong in #44 saying that his was almost done, I'll try to fix the test-failures.
Comment #48
borisson_This should fix the test-failure.
Comment #49
borisson_I rerolled this patch again.
Comment #50
ademarco CreditAttribution: ademarco at Nuvole commentedPatch at #49 works great for me, thanks!
Comment #51
ademarco CreditAttribution: ademarco at Nuvole commentedComment #53
borisson_Committed. Thanks for the reviews / patches.