At the moment we are using a context provider and 1 block to place facets as block.

Some disadvantages of this are:
- No default sane title for a newly created blocks. It's "Facet block" by default, while we could use the facet name instead.
- New workflow, which is a bit strange to grasp (create block, select context)
- This new workflow is not widely used (e.g. when you create a views block display, core creates a separate block for each views block instead of using context providers)
- No context visible if only one facet exists (this is a core bug, and is likely to be fixed in 8.1 or higher, but at this moment its not really acceptible)

Some advantages:
- none I'm aware of.

I discussed this with EclipseGC and Swentel on irc

[23:26:24]  <StryKaizer>	EclipseGc: for Facets we are using contexts to define facets in a block, as you suggested in Barcelona.    I'm wondering what the advantage is compared to creating a block per facet?  (At this moment, it is not that clear which facet is which in the block UI overview, which can be solved by moving to separate blocks instead).    However, if there are good reasons to use context, we'll just need to find another way to fix UX
[23:27:57]  <EclipseGc>	StryKaizer: Ok, so perhaps we miscommunicated, we use contexts when the block requires an object of some sort to operate (and that object is swappable) but something like a particular facet (say date or publish status, etc) could likely be done via plugin derivers
[23:28:06]  <EclipseGc>	StryKaizer: does that make sense when I describe it that way?
[23:31:26]  <StryKaizer>	hmm, not 100% clear, sorry :)
[23:36:09]  <StryKaizer>	EclipseGc: Do you think our implementation (1 block,   context provides all facets which should be selected in the block config) has any advantage over creating a separate block per facet?
[23:37:04]  <EclipseGc>	StryKaizer: I’d have to look at the code
[23:38:00]  <swentel>	StryKaizer, if people create a good name for the block, they're fine no ?
[23:39:28]  <StryKaizer>	swentel: yes, but (and this is core atm, likely to change in 8.1 or so) contexts are not visible if only 1 context exists at this moment
[23:39:39]  <StryKaizer>	swentel: which is just weird for our usecase
[23:40:02]  <swentel>	StryKaizer, yeah, been 'bitten' by that sometimes
[23:40:12]  <swentel>	it's confusing sometimes :)
[23:41:17]  <StryKaizer>	swentel: views creates a block per view, which is kinda similar to our usecase.  I'm guessing there's no real advantage for context for our scenario, which would fix UX (Blocks have a default sane name, now that default is "facet block")
[23:42:25]  <swentel>	StryKaizer, I'd personally vote for block per facet
[23:43:27]  <swentel>	StryKaizer, that was my initial expectation, took me a while to figure out I had to look/search for 'Facet block' when placing the block
[23:43:28]  <StryKaizer>	Same, if we dont loose any advantages I'm unaware of
[23:44:13]  <swentel>	don't know the code enough, but I bet you won't
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

StryKaizer created an issue. See original summary.

StryKaizer’s picture

Issue summary: View changes
StryKaizer’s picture

Issue summary: View changes
borisson_’s picture

I think I agree, it shouldn't be hard to implement with a deriver either.

borisson_’s picture

Status: Active » Needs review
FileSize
9.21 KB

How about this?

borisson_’s picture

FileSize
3.92 KB
10.08 KB

This needed more work.

StryKaizer’s picture

Tested code and worked as expected.

+++ b/src/Plugin/Block/FacetBlockDeriver.php
@@ -0,0 +1,84 @@
+          'admin_label' => $this->t('Facet block: :facet', [':facet' => $facet->getName()]),

I'd use the facet->getName() only here (so without "Facet block:"). This way it has a nice default title already. Core block titles also already have nice useable titles without extra info.

borisson_’s picture

Status: Needs review » Needs work

Sounds good. I'll fix that later and commit.

borisson_’s picture

Status: Needs work » Needs review
FileSize
739 bytes
10.04 KB
borisson_’s picture

Status: Needs review » Fixed

Thanks for the review. Committed.

  • borisson_ committed 436f55b on 8.x-1.x
    Issue #2639560 by borisson_, StryKaizer: Create a separate block per...

  • StryKaizer committed d582eb3 on 8.x-1.x
    Issue #2639560 by borisson_, StryKaizer: Create a separate block per...

The last submitted patch, 6: create_a_separate_block-2639560-6.patch, failed testing.

The last submitted patch, 5: create_a_separate_block-2639560-5.patch, failed testing.

Status: Fixed » Needs work

The last submitted patch, 9: create_a_separate_block-2639560-9.patch, failed testing.

borisson_’s picture

Status: Needs work » Fixed

is already committed, closing up.

Status: Fixed » Closed (fixed)

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