Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/2.x/211 installing blocks #212

Draft
wants to merge 6 commits into
base: 2.x
Choose a base branch
from

Conversation

rupertj
Copy link
Member

@rupertj rupertj commented Apr 26, 2024

Adds a feature that installs default blocks for other LGD modules.

Implements the ideas discussed in #211.

@rupertj
Copy link
Member Author

rupertj commented Apr 26, 2024

There's a few bits of configuration that I want to add to this, but it works. Please take a look and tell me what you think!

I'd recommend starting with the tests: They include a module called localgov_core_default_blocks_test which does nothing but provide a custom block plugin and some configuration to place it using the new mechanism.

@andybroomfield
Copy link
Contributor

Tested by installing a BHCC custom module with a block using the new config/localgov block tecnique

Installs on localgov_scarfolk

Screenshot 2024-04-30 at 11 00 36 AM

and on gov.uk theme

Screenshot 2024-04-30 at 11 00 02 AM

I also tested this using the footer region and the content_bottom region, the later missing from govuk theme and verified it is not placed anywhere and didn't see any error messages. (Maybe it is worth adding a message saying the block still needs to be placed). Perhaps a future enchancement could be that the region key is replaced with a regions array of acceptable regions in order?

I'm happy with this approach, though I have a few questions.

I originally tried to test with Olivero, but that theme uses slightly different names for it's regions (content_below instead of content_bottom). This rasises the issue of wanting to provide default blocks for themes but in different regions, which we could do using the existing method of placing it in config/optional.

I have a question on what to do about Alert banner, which currently does not depend on core currently. Switching to this method would mean that if users did want to use localgov_base and localgov_alert_banner they would also need core to get the default block placement or could I continue to provide one in config/optional? Would that mean two blocks end up getting placed if localgov_core is present? I'm thinking the case that is I provided default blocks under /optional for Olivero and that happens to be the active theme. Maybe a way of either detecting or a module declaring that it will handle installing for that theme itself so this service can skip it.

I also notice that it only works with the active theme, so doesn't effect the admin theme (which is good!). Some site (BHCC) have multiple front end themes where default block placement could be useful, or could also be a hinderance depending on perspective.

Noting there are @todo statements on both those issues above, so it's something we can come back to if we need to move this forward. I'm tempted to actully say it should only target the active theme and so module continue as now with providing default blocks and we skip over the localgov_base / scarfolk.

@rupertj
Copy link
Member Author

rupertj commented May 2, 2024

@andybroomfield I'd like it if people didn't have to do both the existing method of using optional config and the new one. We could support your use case of install into olivero (or another theme with different regions) by using your idea about an array of themes rather than a single value. So if you wanted a block in content_below on olivero, or content_bottom on a localgov theme, you'l list both, and it'd check the regions in order and install into all the localgov themes, plus olivero if it's set as the active theme.

For alert banner, I'd suggest that you add the yml files for this new mechanism and remove the existing config. There's currently only config there for base and scarfolk, and I have a strong suspicion that anyone who's using one of those themes will have localgov_core installed anyway. Anyone who's got a subtheme of base probably won't get the blocks anyway using the current methodology, so nothing's been lost, and at least if they have a subtheme of base and localgov_core installed, they will get them.

@finnlewis
Copy link
Member

Discussing this today in Merge Tuesday, general enthusiasm for the approach.

@Adnan-cds also mentioned the possibility of listing multiple potential regions a block might want to be in.

It'd be good to test this more widely and get this in soon.

@finnlewis finnlewis self-assigned this May 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants