Problem/Motivation

page-content.twig has some hardcoded classes. Causing the need to keep replace the component. The 'page' component is more commonly replaced. This leads to the need to replace both 'page' and 'page-content'.

Steps to reproduce

Proposed resolution

It would be better to move the classes to the page.twig component, as part of the properties. This will improve page-content.twig making it more generic and reusable.

Remaining tasks

User interface changes

API changes

No, existing site should not be affected.

Data model changes

Issue fork radix-3444477

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

ckng created an issue. See original summary.

ckng’s picture

ckng
Primary language English
commented
Status: Active » Needs review

- Removed classes from page-content.twig, moved to page.twig as properties.
- Fixed 'container' property checking. Was not working for `false` and `''` cases, although specified in the schema as part of the `enum`.

ckng opened merge request !96

ckng’s picture

ckng
Primary language English
commented
doxigo’s picture

doxigo commented
Status: Needs review » Fixed

Thanks a lot ckng for the MR, all is valid but I wonder why did we change this part:

{% set page_header_container_classes = [
  page_header_container_type is null ? 'container' : '',
  page_header_container_type ? 'container' ~ (page_header_container_type ? '-' ~ page_header_container_type : '') : '',
  ]|merge(page_header_container_utility_classes ?: []) 
%}

into:

{% set header_container_class = '' %}
{% if page_header_container_type is null %}
  {% set header_container_class = 'container' %}
{% elseif page_header_container_type is not empty %}
  {% set header_container_class = 'container-' ~ page_header_container_type %}
{% endif %}
{%
  set page_header_container_classes = [
    header_container_class
  ]|merge(page_header_container_utility_classes ?: [])
%}

also the same for page_content_container_classes

and if we are to pass page_content_container_type: false, isn't that better to set it to false at the source? also I rather keep the container but to remove it. setting it to false gets rid of it.

I got rid of the py-5 class as you suggested from the page-content and passed it from the page.twig, let me know if this suffice or you think we need more adjustments.
marking this as fixed, feel free to re-open, thanks

  • doxigo committed 57e09710 on 6.0.x 
    Issue #3444477 by ckng: Improve page-content component
ckng’s picture

ckng
Primary language English
commented
Status: Fixed » Needs review

doxigo, the codes changes for `page_header_container_classes` and `page_content_container_classes` are due to setting `page_header_container_type: false` or ``page_header_container_type: ''` were not working.

In the page-content.component.yml:

  page_header_container_type:
      type: ['string', 'boolean']
      title: Container
      description: container type
      default: ''
      enum:
        - false
        - ''
        - sm
        - md
        - lg
        - xl
        - xxl
        - fluid

Since we're supporting `false`, `''`, we're unable to use the `page_header_container_type|default` nor `page_header_container_type is empty` checks, as `null`, `false`, `''` are different. Hence the verbose check on `null` first then `is empty` to cover `false` and `''`. The patch fixes all 3 cases.

  • Hosisam committed 1f700ee4 on 6.0.x 
    Issue #3444477 by ckng: Improve page-content component
hosisam’s picture

hosisam commented

Thanks ckng, Your adjustments make sense in handling cases where page_header_container_type is set to false or an empty string. It's important to have those checks in place to maintain consistency and avoid any unexpected behavior. Overall, I'm satisfied with the changes you've made, and I agree with merging the MR.

hosisam’s picture

hosisam commented
Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

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

danchadwick closed merge request !96