Create new “Views Responsive Grid” format for Views Core

I agree, we basically never use the grid view style in core because of its limitations. Something responsive would be a huge improvement.

Tagging for Views maintainer feedback on whether we can deprecate the row and column classes feature outright, which would be a simpler step than adding the responsive grid display feature (useful in its own right, but more work).

Linking a related issue here. Not sure, it might be a duplicate of this but the issue summary here mentions Olivero whereas that issue is a problem in all core themes (if I remember correctly). But it seems resolving this would resolve that as well, so maybe closing that would be fine?

In my opinion you are much better off adding a new display. Doing the BC dance with the old one will be much more work and updates tend to end up being fragile.

And this being Views, I'm certain somebody has cooked up something weird with the settings we think are useless and should be removed.

With all tests passing, including a new unit test for the plugin added, moving this to Needs Review.

I tested on live preview, both grid view horizontal and grid view vertical. Added the screen-recording for reference. views grid styles are responsive.

Can be move to RTBC.

I forgot to reply here, myself, but I gave it a look over and thumbs up from me. This is exciting!

Gave the MR quick peek and it looks awesome! I hope that we get this in soon 🎉

I think this is a bit more than normal.

Should we also look into deprecating the non-responsive grid eventually, or are there still use-cases for that?

I bet there's use-cases and deprecating the format users need to know how to replace it

I'm removing 'needs subsystem maintainer review' since @lendude has commented on here.

To actually mark the existing grid deprecated, we'll need #3039240: Create a way to declare a plugin as deprecated which is not in yet.

I'm going to tag this for product manager review - are we OK to add a new plugin without deprecating the old one?

I think its ok to add a new one without depreacting the old one, since the features are not 1-1. In case of the existing core grid, the columns setup stays the same regardless of viewport. I would not suggest anyone to use the non-responsive grid and I would not add that feature to core these days, but it is already there. It is important to gently deprecate it with proper tooling and I don't think this needs that deprecated to be added.

#26 makes sense to me - we can open a follow-up to discuss deprecating the old grid layout once this has landed.

It looks amazing on tugboat.

I'd like to see the test coverage expanded here, we are only testing with 1 set of values and it would be nice to run some different sets. Also the defaults aren't being tested currently.

I would suggest rewriting the test to use a @dataProvider so it becomes easier to test different data sets (one of which could be empty to test the defaults).

Checked out the CSS, my only hesitation is the negative margin to bring the bounding box in line with the end of the bottom row for the vertical grid because it will likely surprise newer themers.... but then I realised the other option was fielding hundreds of "how do I remove this gap?" questions which is way harder to understand than why a gap is bigger than you intended after setting a margin.... so in short: no further comment on the CSS

Responding to some earlier feedback:

We can get rid of this $alignmentsTested variable and any references to it. It is in the StyleGridTest because $this->assertGrid is called multiple times on each alignment. In this new StyleGridResponsiveTest, $this->assertGrid is only called once per alignment.

There's no value in including it because it's only used twice? I'm not sure I follow the logic there. Also, the test associated with this property does validate that the expected class is being applied. That speaks to this comment:

Could you add an assertion to confirm that the items are being rendered with the right markup?

And to respond to the comment by @Lendude in #31:

I'd like to see the test coverage expanded here, we are only testing with 1 set of values and it would be nice to run some different sets. Also the defaults aren't being tested currently.

I would suggest rewriting the test to use a @dataProvider so it becomes easier to test different data sets (one of which could be empty to test the defaults).

I'm not against trying to improve what we have here, but I think it's worth pointing out that what is here already is equivalent to what has already been implemented (and accepted into core) for the existing grid view. Could we accept what's here and open a child issue for the expanded test coverage?

There's no value in including it because it's only used twice? I'm not sure I follow the logic there.

Here's what I mean. The new test clearly began as a copy/paste StyleGridTest.php. That test ran assertGrid for many more configurations:

  public function testGrid() {
    $view = Views::getView('test_grid');
    foreach (['horizontal', 'vertical'] as $alignment) {
      $this->assertGrid($view, $alignment, 5);
      $this->assertGrid($view, $alignment, 4);
      $this->assertGrid($view, $alignment, 3);
      $this->assertGrid($view, $alignment, 2);
      $this->assertGrid($view, $alignment, 1);
    }
  }

Some of the assertions only need to be done once per alignment. That's why StyleGridTest.php. has $alignmentsTested: to prevent running redundant assertions.

For the new test, there's only one horizontal configuration and one vertical configuration:

  public function testGrid() {
    $view = Views::getView('test_grid_responsive');
    foreach (['horizontal', 'vertical'] as $alignment) {
      $this->assertGrid($view, $alignment);
    }
  }

So there aren't going to be redundant assertions and we don't need $alignmentsTested.

But we can't simply remove the declaration of $alignmentsTested. We also have to modify

    // Validate test values.
    if (!in_array($alignment, $this->alignmentsTested)) {
      $result = $this->xpath('//div[contains(@class, "views-view-responsive-grid") and contains(@class, :alignment)]', [':alignment' => 'views-view-responsive-grid--' . $alignment]);
      $this->assertGreaterThan(0, count($result), ucfirst($alignment) . " grid markup detected.");
      $this->alignmentsTested[] = $alignment;
    }

such that any logic related to $alignmentsTested is removed.

Alternatively, we could add more cases to testGrid() and then $alignmentsTested would be useful.

@danflanagan8 you are spot on that the new test is adapted from the StyleGridTest.php. Unfortunately, writing unit tests isn't one of my strengths, so being able to leverage existing code is a big help. Can you recommend one or more tests elsewhere in core that I could use as examples to write the additional tests / cases needed to provided sufficient coverage here?

After communicating with @mandclu in Slack, I decided to take off my Reviewer Hat and put on my Test Writing Hat. I have pushed some updates that address my concerns with the tests as well as the comments from @lendude in #31.

I also did a rebase where I had to resolve a conflict in Stable9LibrariesOverrideTest. I think I got it right, but the git history looks kind of funky for the rebase. Hopefully I didn't mess anything up. I really struggle with the MR workflow.

Anyone want to review? :)

Thank you, @mherchel

For some reason, Gitlab said this had a merge error.

I must have done something wrong with the rebase I attempted. I'm very thankful you were able to fix it.

Hopefully we can get this across the finish line! As far as I know, the only outstanding task was to enhance the test coverage, which I have addressed. I will be happy to make additional changes to the test coverage if they are requested.

Removing the 'Needs change record' since that was from a time when we were still thinking about deprecating the old grid here. That has been moved to the follow up so removing the tag here.

Nice to see the discussion on the possible test coverage, interesting to see what happens there.

I don't see anything else to nitpick, so moving to RTBC

fwiw I've been running this patch on a site for 3-4 months with no problems (except keeping up with re-rolls). Not doing anything extreme with it, but it's working very well.

Committed 6aed2e9 and pushed to 10.1.x. Thanks!

Leaving open for a backport to 10.0.x once the freeze is over. I don't think this can be backported to 9.x given that this is fundamentally not compatible with IE 11.

Cherry-picked to 10.0.x.

Thank you so much @mherchel, @mandclu, @danflanagan8, @catch, @Lendude, @lauriii and @Gábor for making this great improvement. It looks amazing. Especially the vertical style with the images automatically resizing and realigning looks almost magical :-)