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

597 Don't render regions up-front to see if they are empty #601

Draft
wants to merge 1 commit into
base: 1.x
Choose a base branch
from

Conversation

erikerskine
Copy link

What does this change?

Does not render regions in localgov_base_preprocess_page() when determining things like has_sidebars.

A region is now considered non-empty if it one or more blocks are placed within it (and those blocks are configured to be visible).
We do not consider what those blocks will actually output (if anything).
If a block is placed but produces no output, its region is still considered to be non-empty.

This is closer to Drupal's core behaviour and is necessary for placeholdering and BigPipe to work properly.

See #597 for discussion

How to test

  1. Add a block that produces no content. (This can be a custom block plugin, a view, anything)
  2. Place that in a sidebar. Make sure it is the only thing in the sidebar.
  3. The page should render with a sidebar.

How can we measure success?

An otherwise cacheable page with an uncacheable block in a sidebar will now be cacheable.

Have we considered potential risks?

Sites which rely on this to hide sidebars when blocks do not produce content may find that those sidebars now appear.

That can be mitigated against, with:

  • visibility conditions on the block configuration page
  • using CSS :has to detect emptiness on the client side
  • a contrib module to force blocks to render ahead of time

Accessibility

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.

1 participant