Hello!
Suggestion: why not use the medium breakpoint instead of the large in zurb_foundation.theme lines 294-309?
I believe that there is still room for having the sidebars aside of the main content on let's say an iPad in portrait orientation, now they are pushed down below the main, just as on a smartphone. That's because the large breakpoint is used. Here is the code with the medium replacing the large.

if (!empty($left) && !empty($right)) {
    $variables['main_grid'] = 'medium-6 medium-push-3';
    $variables['sidebar_first_grid'] = 'medium-3 medium-pull-6';
    $variables['sidebar_sec_grid'] = 'medium-3';
  }
  elseif (empty($left) && !empty($right)) {
    $variables['main_grid'] = 'medium-9';
    $variables['sidebar_first_grid'] = '';
    $variables['sidebar_sec_grid'] = 'medium-3';
  }
  elseif (!empty($left) && empty($right)) {
    $variables['main_grid'] = 'medium-9 medium-push-3';
    $variables['sidebar_first_grid'] = 'medium-3 medium-pull-9';
    $variables['sidebar_sec_grid'] = '';
  }
  else {
    $variables['main_grid'] = 'medium-12';
    $variables['sidebar_first_grid'] = '';
    $variables['sidebar_sec_grid'] = '';
  }

For me is was a bit hard to find that here is where I could change this behaviour.
Thanks for a great base theme!

Comments

hstrindb created an issue. See original summary.

bandanasharma’s picture

Created patch for this. Also attached the interdiff file.

samuel.mortenson’s picture

Status: Active » Needs review
serg2’s picture

Status: Needs review » Reviewed & tested by the community

The patch still applies against the dev and is a sensible change.

hongpong’s picture

In my experience this is a good idea and I am inclined to commit it, unless there are objections.

serg2’s picture

This patch makes the theme more responsive and is a good change. There have not been any objections so lets roll it in before the stable :)

hongpong’s picture

Status: Reviewed & tested by the community » Fixed

Good call. Thanks bandanasharma and serg2, it is now in 8.x-6.x . I do not think it should be put on 8.x-5.x because that is already in beta and it would be a breaking change, but I would be open to a counter argument if needed.

Status: Fixed » Closed (fixed)

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