Problem/Motivation

Currently each DROWL Layout hast statically defined layout/breakpoint classes (small-12, medium-6, etc.).
In many situations this is really bad and inflexible.

Example:

Case A) 3 column layout, each cell has a video, lets say those videos should have a minium width of ~300px, so all controls are still clickable and the video doesn't become to small at all. So the cells should break already on all breakpoints smaller than large (currently this will break on screens smaller than medium).

Case B) 3 column layout, each cell with a simple icon in it ~32px width. This will also break on screens smaller than medium, what works, but looks crappy and wastes a lot of space.

Proposed resolution

Add a new layout called "Dynamic Content Grid". Technically this is a layout with only one region, the layout will be created from direct childs of this region by a CSS Grid or/and Flexbox. Using the child blocks/paragraphs as layout cells is a good thing at all in many situations and very flexible.

This layout needs the following options:
- Our layout regular alignment settings
- Class field
- Cell with by breakpoint (so one select for each breakpoint), values:
-- "unset"/"-"/"none" ...
-- Grid Column 1 ... 12
-- Automatic (fit content)
-- Min-Content
-- Max-Content
- Grid Gutter Width (select)
-- "default" => foundation grid gutter width
-- xxs - xxl (drowl base theme spacing classes)

Remaining tasks

[x]We need to define the layout, with the settings defined above.
[x] Review & optimize the PHP code: https://git.drupalcode.org/issue/drowl_layouts-3307235/-/commit/dd473283...
[x] Provide a Drupal library for "dynamic grids"
[x] Add required (S)CSS
[x] Add the required CSS classes in the layout template

CommentFileSizeAuthor
#19 drowl_layouts_dynamic_layout_options.png32.12 KBthomas.frobieter

Issue fork drowl_layouts-3307235

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

thomas.frobieter created an issue. See original summary.

thomas.frobieter’s picture

thomas.frobieter
Primary language German
Location EU
commented
Issue summary: View changes
thomas.frobieter’s picture

thomas.frobieter
Primary language German
Location EU
commented
Issue summary: View changes
thomas.frobieter’s picture

thomas.frobieter
Primary language German
Location EU
commented
Related issues: +#3299803: Add columns per breakpoint setting for single columns
thomas.frobieter’s picture

thomas.frobieter
Primary language German
Location EU
commented

Case 2) is basically identical to: https://www.drupal.org/project/drowl_layouts/issues/3299803 - I'll update the issue.

thomas.frobieter’s picture

thomas.frobieter
Primary language German
Location EU
commented
Title: Dynamic Layout » Dynamic Content Grid
Issue summary: View changes
thomas.frobieter’s picture

thomas.frobieter
Primary language German
Location EU
commented
Issue summary: View changes
thomas.frobieter’s picture

thomas.frobieter
Primary language German
Location EU
commented
Issue summary: View changes
thomas.frobieter’s picture

thomas.frobieter
Primary language German
Location EU
commented

Forgot to switch the branch.. accidentally pushed to dev. So ignore the issue fork.. We need to finish this soon, before the next release.

See this commit: https://git.drupalcode.org/issue/drowl_layouts-3307235/-/commit/dd473283...

Its technically working, someone please need to review the PHP part, especially src/Plugin/Layout/DrowlLayoutsDynamicContentGridLayout.php.

My understanding of this trait .. stuff is not that good. We dont need the column_width setting from DrowlLayoutsMultiWidthLayoutBase, so we might need to extend another class here? I don't know.. maybe DrowlLayoutsLayoutDefault?

Afterwards we need to remove the $columnCount, getColumnCount and getWidthOptions stuff.

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented
Status: Active » Needs review

Here's the review:

  function drowl_layouts_preprocess_layout(&$variables) {
  // Attach Libraries for specific layouts
  // - Layout "Dynamic Content Grid"
  if(!empty($variables['theme_hook_original']) && $variables['theme_hook_original'] == 'drowl_layout__dynamic_content_grid'){
    $variables['#attached']['library'][] = 'drowl_layouts/dynamic_grid';
  }

$variables['theme_hook_original'] is untypical and I guess prone to errors. Have you seen alternatives for this?

This part is definitely wrong in the trait and should be in src/Plugin/Layout/DrowlLayoutsDynamicContentGridLayout.php extending the existing settings, but only for this layout:

      // Dynamic Layout settings
      'layout_dynamic_grid_type_sm' => 'auto',
      'layout_dynamic_grid_type_md' => 'auto',
      'layout_dynamic_grid_type_lg' => 'auto',

// TODO: Is this required? We might need to extend another class
protected $columnCount = 2;

Indeed seems incorrect here, as we have no columns, so this has to be reworked.

I'll work on this later.

Warning: Using the branch an saving ANY layout based on DrowlLayoutsSettingsTrait before this is fixed, may store wrong settings and break things after this was fixed.
So better don't use dev at all anywhere, currently!

@thomas.frobieter could you push the changes to the MR instead and roll them back in dev so we can proceed here cleanly?

thomas.frobieter’s picture

thomas.frobieter
Primary language German
Location EU
commented
Title: Dynamic Content Grid » Add new Layout: Dynamic Content Grid
Issue summary: View changes
thomas.frobieter’s picture

thomas.frobieter
Primary language German
Location EU
commented

@Anybody: Let us change the name of the settings layout_dynamic_grid_type_* to layout_dynamic_grid_column_sizing_* more ..meaningful I guess.

thomas.frobieter’s picture

thomas.frobieter
Primary language German
Location EU
commented

$variables['theme_hook_original'] is untypical and I guess prone to errors. Have you seen alternatives for this?

Mhm.. yeah there is a 'layout' subarray which have the name available very likely.

---

When you make the changes, please ensure we still have the alignment options available (so exactly like it is for simple one column layouts).

Thank you so much!

Anybody opened merge request !6

Anybody closed merge request !6

Anybody reopened merge request !6

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented

@thomas.frobieter finished refactoring. Hope this should work now! :)

Please test in detail and carefully.

thomas.frobieter’s picture

thomas.frobieter
Primary language German
Location EU
commented

I think we need to rethink this solution. For all the "SM/MD/LG x of 12" column settings we would need to generate dynamic css code, with PHP/Twig to have the themes media queries available.

This adds a lot of complexity here and furthermore this was not really the idea of this layout (even if it where a good thing, because columns are created dynamically out of the paragraphs/contents without any layout-region limitations).

However, the new idea is, to make use of the CSS minmax(), min(), max(), min-content, max-content, ... to really make this an dynamic grid. For breakpoint driven layout definitions, we stick with the regular (Foundation) layouts.

This way, we only need two text fields (min-width, max-width - both optional), no breakpoint differentiation. So we basically say "This cell should be xPX minimum (and xPX maximum width)".

Those values will be simple inline CSS for each of this layout instances, no breakpoint struggle at all.

I will play around with this idea ...

thomas.frobieter’s picture

thomas.frobieter
Primary language German
Location EU
commented
StatusFileSize
new drowl_layouts_dynamic_layout_options.png32.12 KB

I am very happy with the results, ended up with this options:

I'll finish the CSS code and push this great feature😁

thomas.frobieter’s picture

thomas.frobieter
Primary language German
Location EU
commented
Assigned: thomas.frobieter » grevil

Done! To make this more userfriendly, we might need something to visualize pixel values. This might be a JS input field showing a rectangle with the given width or some simple static example like (rectangle with 100px width and a "100px" label inside). But we can add this later.. this feature is basically for experienced users.

@grevil please have a look at the PHP part (:

thomas.frobieter’s picture

thomas.frobieter
Primary language German
Location EU
commented
Issue summary: View changes

Grevil made their first commit to this issue’s fork.

grevil’s picture

grevil commented
Status: Needs review » Reviewed & tested by the community

Fixed a few tiny things and checked for phpcs errors!

Other than that, LGTM!

EDIT: FYI, make sure to work on the issue fork branch next time! 🙂👍

Grevil opened merge request !7

Grevil closed merge request !7

grevil’s picture

grevil commented
Issue summary: View changes
thomas.frobieter’s picture

thomas.frobieter
Primary language German
Location EU
commented
Status: Reviewed & tested by the community » Fixed

Thanks! Release incoming ;)

EDIT: FYI, make sure to work on the issue fork branch next time! 🙂👍

Not my fault ;) startet do work on the issue branch.. then I realized, that the other changes where made on 4.x. Furthermore - if I remind correctly - @Anybody told me, this isn't very relevant. Correct me if I'm wrong @Anybody.

thomas.frobieter’s picture

thomas.frobieter
Primary language German
Location EU
commented
Status: Fixed » Needs work

God ******* something went wrong here. Changes aren't merged and I am not able to crate a new MR, because of "Validate branches Another open merge request already exists for this source branch: !6".

thomas.frobieter’s picture

thomas.frobieter
Primary language German
Location EU
commented

So .. found a nearly invisible merge-select on the bottom here, still great UX.
However.. lets see if I finaly broke everything here ;)

thomas.frobieter’s picture

thomas.frobieter
Primary language German
Location EU
commented
Status: Needs work » Fixed

Relase done! Thank you guys.

thomas.frobieter’s picture

thomas.frobieter
Primary language German
Location EU
commented
Status: Fixed » Needs work

Got the following issues on a customer page while configuring this layout in a layout paragraph:

Warning: Undefined array key "layout_dynamic_grid_behavoir" in Drupal\drowl_layouts\Plugin\Layout\DrowlLayoutsDynamicContentGridLayout->buildConfigurationForm()

Same for the other fields:

Warning: Undefined array key "layout_dynamic_grid_col_min_width"

Warning: Undefined array key "layout_dynamic_grid_col_max_width"

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented

@thomas.frobieter: @Grevil is still assigned, should he pick up the errors and fix them?

grevil’s picture

grevil commented

Sorry guys, seems that I skipped the email...

grevil’s picture

grevil commented

We haven't set any default values, that's why the errors appear.

grevil’s picture

grevil commented

Yep, setting the default configurations fixes this issue, but I have no idea what the defaults should be...

@thomas.frobieter, please tell me the default configurations for:

  public function defaultConfiguration() {
    return parent::defaultConfiguration() + [
      // Add dynamic Layout settings:
      'layout_dynamic_grid_col_min_width' => 10, // ?
      'layout_dynamic_grid_col_max_width' => 10, // ?
      'layout_dynamic_grid_gutter_size' => 'md',
      'layout_dynamic_grid_behavoir' => 'auto-fit', // ?
    ];
  }

  • Grevil committed 6aaa7629 on 4.x 
    Follow up fix to: Issue #3307235 Add new Layout: Dynamic Content Grid
grevil’s picture

grevil commented
Status: Needs work » Fixed

Alright, talked with @thomas.frobieter internally. The defaults are fine. I'll create a new release ASAP!

Status: Fixed » Closed (fixed)

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