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

Stops blocks that are assigned to the disabled region being rendered. #498

Merged
merged 4 commits into from
Nov 20, 2023

Conversation

rupertj
Copy link
Member

@rupertj rupertj commented Nov 1, 2023

Added 'disabled' region to the list of regions not to render. Limited region list to visible ones. Moved list of excluded region names out of the loop and simplified vars in the loop.

Fixes #497.

… region list to visible ones. Moved list of excluded region names out of the loop and simplified vars in the loop.
@finnlewis
Copy link
Member

Maybe want testing to confirm and approve

@rupertj
Copy link
Member Author

rupertj commented Nov 14, 2023

So I've added a test for this change. Turns out it was pretty hard to test...

There's a couple of fails showing up, but they're caused by this: localgovdrupal/localgov_project#144, not the code in this PR.

@rupertj
Copy link
Member Author

rupertj commented Nov 15, 2023

Tests are fully green again. What do you reckon @finnlewis?

@finnlewis
Copy link
Member

@ekes is taking a look

Copy link
Member

@ekes ekes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll say this is makes sense. The test checks as expected.

Would to be good to know about the REGIONS_VISIBLE / 'disabled' thing though.

* We do all this because the page render runs in a different process to this
* test, so we can't just read the value from the class directly.
*
* @dataProvider blockRegionProvider
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK so this is testing if something is rendered and something isn't rendered. That's pretty much all we need to do here. And as each run is a separate test the static value setting the header will be different and correct.

$has = 'has_' . $region;
$excluded_regions = [
'messages',
'disabled',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does disabled get returned with REGIONS_VISIBLE set on the request? Are there disabled not visible regions? What are they for?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In a theme's info.yml file, there's an optional key called 'regions_hidden', which the docs describe as 'A list of inherited regions to remove.'. If you pass the REGIONS_ALL constant to system_region_list(), these get returned. If you pass REGIONS_VISIBLE, they don't.

So my understanding of this is that it's a mechanism to allow a theme to not inherit a region from its base theme, and we shouldn't try to do anything with regions that haven't been inherited.

@finnlewis finnlewis merged commit 2ee7bdc into 1.x Nov 20, 2023
11 checks passed
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.

Disabled blocks get rendered unnecessarily
3 participants