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

Replace is_panelized by has_page_layout variable. #36

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

Conversation

kybersoft
Copy link

@kybersoft kybersoft commented Nov 12, 2018

Hi all,

as a reaction on issue govCMS/GovCMS8#133 I prepared more generic solution than current "is_panelized" variable, so theme is now completely without relation to word "panel".

It's based on Layout Discovery module preprocess, so it works for all 4 possible page setup situations (Display Suite, Field Layout, Layout Builder and Panelizer). All tested separately.

Implementation details:

  • Layout Discovery module provides preprocess for all layouts used on page.
  • We can set flag into static variable storage if any GovCMS8 Layouts page layout is used.
  • Then we can get this information inside preprocess_page (it's always running after layouts).
  • We can set template variable with boolean as before (renamed to "has_page_layout").
  • Also replaced all usages of "is_panelized" variable and "is-panelized" class.

Could this be merged? Thank you.

@kybersoft kybersoft changed the title Replace is_panelized by is_page_layout variable. Replace is_panelized by has_page_layout variable. Nov 12, 2018
@murraywoodman
Copy link

To add some context... this PR will mean that Panelizer can be swapped out by site builders who would prefer to use an alternate system: Layout Builder, Display Suite or Field Layout. In the future we see a move to Layout Builder but some people may just want to return back to Field Layout.

Now that the variable is more generic it means that the page layouts which are defined in govcms_layouts will be deemed to take up the full width. ie. the page template will not be putting in a container div limiting their width. This means that full page layouts which make use of Paragraphs and Modifiers will still have the ability to get backgrounds to go edge to edge.

The way has now been opened to swap out Panelizer for the full content view mode without any loss of functionality.

As Vit says, this new code has been tested against 4 site installs each using a different paradigm for handling the fields.

@petrillek
Copy link
Contributor

petrillek commented Nov 13, 2018

I've tested the PR for all four layout systems (core Layout Field, Layout Builder, Display Suite and Panelizer) and all the layouts looks the same in each of this system. So it will be easy to remove Panelizer and replasce it with different layout mechanism and still use all the provided layouts with their logic.
I would say it is safe to merge.

@murraywoodman
Copy link

This improvement dramatically increases the utility of the theme by making it work for site builders who do not wish to use the Panels ecosystem. The theme and foundations will get a 3x flexibility boost if it can be merged. Ideally it would get in before the initial release.

@tobybellwood tobybellwood added the Needs review Pull requests needs a review from assigned developers label Dec 8, 2019
@tobybellwood
Copy link

tobybellwood commented Dec 8, 2019

Hi @kybersoft - sorry it's taken so long to get to this. Can you have a look now that we've updated the 8.x-1.x codebase - there's a merge conflict in style.css (easy fix obviously), but of more concern is that there are now two competing govcms8_uikit_starter_preprocess_layout functions in the theme - can they be easily merged and re-tested?

@tobybellwood tobybellwood added the invalid This doesn't seem right label Dec 8, 2019
@kybersoft
Copy link
Author

Hi @tobybellwood , I merged the main branch and fixed the duplicated function (combined both parts together). Now it looks fine from my point of view.

@tobybellwood tobybellwood changed the base branch from 8.x-1.x to 2.x April 28, 2020 01:31
@tobybellwood
Copy link

Thanks @kybersoft - I've just rebased this onto 2.x, with an intent to release it soon. Does the has_page_layout change have any requirements on the distro to enable (or will it break existing implementations at all?)

@kybersoft
Copy link
Author

Hi @tobybellwood , I checked your rebased version, improved a little bit, and added also the old variable name for backward compatibility, so we can be sure it doesn't break existing implementations.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
invalid This doesn't seem right Needs review Pull requests needs a review from assigned developers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants