Quick review on patch #4:
Add a new "add block" checkbox for a facet in the form and the config and only show the facet in the available contexts (\Drupal\facetapi\ContextProvider\FacetContextProvider::getAvailableContexts) when the add block checkbox is active.

This needs changes in:
- \Drupal\facetapi\ContextProvider\FacetContextProvider::getAvailableContexts
- facetapi\config\schema\facetapi.facet.schema.yml
- \Drupal\facetapi\Form\FacetForm

Maybe the check for deletion can be simplified in isFacetUsedBlock in \Drupal\facetapi\Form\FacetDeleteConfirmForm.

CommentFileSizeAuthor
#4 2596331-add-message-only.patch2.02 KBborisson_
#2 add_a_add_block-2596331-2.patch4.38 KBborisson_
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

borisson_ created an issue. See original summary.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer commented
Status: Active » Needs review
FileSize
add_a_add_block-2596331-2.patch4.38 KB

Added a patch

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer commented

There is no additional overhead to just having the context offer all possible facets to the block UI, so I'm not sure if we should include this.
I'd prefer not to have this patch in because it adds unneeded ties to the block system and I don't think the additional overhead created by having all facets returned from \Drupal\facetapi\FacetContextProvider::getAvailableContexts is enough to include this.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer commented
FileSize
2596331-add-message-only.patch2.02 KB

I think it should be sufficient to just add a message about this in the UI, see attached patch.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer commented
Status: Needs review » Needs work

It looks like @StryKaizer agrees with the patch in #4 but we'll probably have to rewrite it a bit.

This is from a conversation in irc before StryKaizer got distracted.

13:33 borisson_: StryKaizer: if you have like 5 minutes an opinion on https://www.drupal.org/node/2596331 would be very much appreciated.
13:44 StryKaizer: borisson_ : why would we need an explicit checkbox for this and not just list all facets in the block context listing?
13:45 StryKaizer: borisson_ : looks like you have the same remark :) sry, didnt read the comments
13:47 StryKaizer: borisson_ : I can live with a message only, maybe rephrase it a bit, lets see..

Setting this issue to NW, someone can rephrase the message in from the patch in #4.

Novitsh’s picture

Novitsh CreditAttribution: Novitsh at Colruyt Group Services commented
Issue summary: View changes 
+++ b/src/Form/FacetForm.php
@@ -533,6 +534,13 @@ class FacetForm extends EntityForm {
+        $message = $this->t('A new context for blocks is automatically created. Go to the <a href="@block_overview">Block overview page</a> and add a new "Facet block". If this is your first and only facet, just adding that block make it link to this facet, if you have addded more facets already, please make sure to select the correct Facet to render.', ['@block_overview' => \Drupal::urlGenerator()->generateFromRoute('block.admin_display')]);

Should we not go for the :placeholder instead of @placeholder as marked in https://www.drupal.org/node/2571689 ?

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer commented
Status: Needs work » Fixed

Made the change as remarked by @Novitsh in #6. We can always change the text later.

  • borisson_ committed fc51e96 on 8.x-1.x 
    Issue #2596331: Add a 'Add block' checkbox for a facet

Status: Fixed » Closed (fixed)

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