I have made a Grid display in my view. It is set to 4 columns, horizontal.

But it doesn't seem to display as a Grid, either in Preview or in the Bartik theme.

Before patch in #62

After patch in #62

Comments

jhodgdon’s picture

Title: View grid style doesn't make a grid » Regression: View grid style doesn't make a grid
Issue tags: +VDC
jhodgdon’s picture

Status: Active » Closed (cannot reproduce)

This vanished. ??

mpotter’s picture

Status: Closed (cannot reproduce) » Active

I'm having this same issue. Fresh install of beta12. Created a Grid view. Displays an image field using an image style. Set to 4 columns.

The nodes (images) each show on separate lines, rather than all in a row. This worked fine in beta9 (yeah I know, it's been a while since I've been able to do D8 testing).

Looking at the markup, the columns are set to 25%, but have "display: block". Changing this to "display: inline-block" fixes the problem. But not sure where to apply this change in D8.

cilefen’s picture

Priority: Normal » Major

I tried it too.

cilefen’s picture

I am looking for the commit that may have done this. It could be #2329917: Move views classes from preprocess to templates but that is just an early guess.

cilefen’s picture

Issue tags: +Twig
cilefen’s picture

I have looked back as far as beta8 and the regression is there.

dawehner’s picture

Issue tags: +Needs tests

Meh, I hate if things randomly breaks. Let's ensure that this doesn't happen in the future.

dpi’s picture

Grids were working on page displays in beta11, but not in admin preview

dawehner’s picture

Does that mean we also somehow miss to add CSS files in preview?

dpi’s picture

The earliest version of D8 I have on this machine is beta2, page views work, preview does not work.

An up to date version of D7 + Views have both operating correctly.

Perhaps the admin preview was not ported completely for D8? The page view issue is recent as I generated the image on the Avatars project page with grid view on beta11.

davidhernandez’s picture

It looks like we're getting the right classes. views-view-grid horizontal cols-4 clearfix

Did we lose some CSS at some point?

Should this be critical? I don't think we want to ship with broken displays.

davidhernandez’s picture

/* Grid style column align. */
.views-view-grid .views-col {
  float: left;
}

views.module.css is not being loaded.

cilefen’s picture

On beta 7, Bartik works but not Seven.

cilefen’s picture

This has been broken a while - I am back to beta 5 and it is the same thing as beta 7: Bartik works but not Seven

davidhernandez’s picture

The code to attach the library is in ViewExecutable.php

  public function __construct(ViewEntityInterface $storage, AccountInterface $user, ViewsData $views_data, RouteProviderInterface $route_provider) {
    // Reference the storage and the executable to each other.
    $this->storage = $storage;
    $this->storage->set('executable', $this);
    $this->user = $user;
    $this->viewsData = $views_data;
    $this->routeProvider = $route_provider;

    // Add the default css for a view.
    $this->element['#attached']['library'][] = 'views/views.module';
  }

Is this not executing?

The library itself is registered fine. I can force it to be added directly in a theme info file.

cilefen’s picture

Beta 1 is the same thing as beta 7 - I am not looking back any more.

cilefen’s picture

Maybe DisplayPluginBase::buildBasicRenderable() should be attaching the library from the element in ViewExecutable.

cilefen’s picture

Status: Active » Needs review
FileSize
733 bytes
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,268 pass(es), 8 fail(s), and 4 exception(s). View

This is a test to see if I am in the right place. This fixes it for Bartik. This is not how one would really fix it.

Status: Needs review » Needs work

The last submitted patch, 19: regression_view_grid-2529748-19.patch, failed testing.

cilefen’s picture

Status: Needs work » Needs review
FileSize
1.32 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,281 pass(es), 8 fail(s), and 4 exception(s). View

This fixes it in Seven and Bartik. Again, this is not an elegant solution.

Status: Needs review » Needs work

The last submitted patch, 21: regression_view_grid-2529748-21.patch, failed testing.

Chernous_dn’s picture

Status: Needs work » Needs review
FileSize
782 bytes
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,279 pass(es). View
135.65 KB

I think need to attached styles in views.theme.inc in function template_preprocess_views_view_grid

Chernous_dn’s picture

FileSize
14.53 KB

And screenshot in preview.

davidhernandez’s picture

I don't think this is actually a grid problem. Views is not attaching the views.module library. It is manifesting itself with the grid, because the only CSS in that file is for table and grid displays.

cilefen’s picture

Title: Regression: View grid style doesn't make a grid » Regression: Views module CSS is not attached
Chernous_dn’s picture

FileSize
2.85 KB
1.07 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,233 pass(es). View

Update patch #23 add attached for table.

Chernous_dn’s picture

FileSize
136.9 KB

Add screenshot.

dawehner’s picture

+++ b/core/modules/views/views.theme.inc
@@ -670,6 +670,8 @@ function template_preprocess_views_view_table(&$variables) {
+  // Attached style for table.
+  $variables['view']->element['#attached']['library'][] = 'views/views.module';

@@ -771,6 +773,8 @@ function template_preprocess_views_view_grid(&$variables) {
+  // Attached style for grid.
+  $variables['view']->element['#attached']['library'][] = 'views/views.module';

I'm a bit confused, should not the actual preprocess_views_view add the library?

Chernous_dn’s picture

@dawehner may be styles added once in template_preprocess_views_view ? Or in template_preprocess_views_view_table and template_preprocess_views_view_grid attached twice them when they are needed.

davidhernandez’s picture

Yeah, what I was trying to get across in #25 is that this isn't just about the grid. Attaching the library in that construct() doesn't work. I don't know enough about Views to know if it should work, or we should be moving it elsewhere. Maybe the theme system could once process attachments at that level, but now can't? I don't know. But if the attachment line is moved, it should be someplace general, like preprocess_views_view.

I suppose it could live in _view_grid and _view_table if we are ok with saying that this css only matters for those two and don't want it getting include for others. But, in that case, we should probably change the name of the library.

Chernous_dn’s picture

@davidhernandez I tested and add to template_preprocess_views_view and it works fine. But it is necessary to do it?

dawehner’s picture

I suppose it could live in _view_grid and _view_table if we are ok with saying that this css only matters for those two and don't want it getting include for others. But, in that case, we should probably change the name of the library.

Yeah exactly. Maybe someone adds some generic CSS and then we basically end up with the same bug as this issue again.

Chernous_dn’s picture

FileSize
755 bytes
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,226 pass(es), 4 fail(s), and 4 exception(s). View

@dawehner update patch #27 attached style in template_preprocess_views_view tested works fine. Probably it would be more correct.

Chernous_dn’s picture

FileSize
126.21 KB

Add screenshot.

Status: Needs review » Needs work

The last submitted patch, 34: regression_views-2529748-34.patch, failed testing.

Chernous_dn’s picture

Status: Needs work » Needs review
FileSize
736 bytes
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,232 pass(es), 4 fail(s), and 4 exception(s). View

Update patch #34.

dpi’s picture

FileSize
544 bytes
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,231 pass(es), 4 fail(s), and 4 exception(s). View

This is a different path, much simpler.

Turns out $view->element was being assigned a new value. This path simply merges in defaults.

Cause #2381277: Make Views use render caching and remove Views' own "output caching" (15c848bd6)

cilefen’s picture

@dpi That is the problem I was trying to overcome in #21. #38 is simpler.

The last submitted patch, 37: regression_views-2529748-37.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 38: 2529748-missing-views-css.patch, failed testing.

davidhernandez’s picture

The fail is because the json response data is now missing the add_css command.

    [1] => Array
        (
            [command] => add_css
            [data] => <link rel="stylesheet" href="http://d8.localhost/core/modules/views/css/views.module.css?nrfuxk" media="all" />

        )

...but this is ok, right? Since the css is actually already in the page, it doesn't need to get added again. If so, we just need to update the ViewAjaxTest test.

To actually test this change we can put an assert looking for the css file, instead of creating a new test, if there is some place generic to do it. Look for a plain "Does this View display" web test.

Chernous_dn’s picture

Status: Needs work » Needs review
FileSize
36.7 KB

Tested patch it does not work for view (add screenshot). So may be patch is correct?

dpi’s picture

It works, clear your cache.

Chernous_dn’s picture

@dpi I clear cache, but still not work for edit views.

dpi’s picture

Three issues that need an answer:

  1. Before the patch is applied, it looks like somehow the AJAX request is getting instructions to add the CSS:
    1. But the JS does not add it to the page, see admin previews
    2. CSS is getting discovered for AJAX, but not Pages.
  2. I thought I would fix the issue by merging the variables, but the CSS then disappears from the AJAX response.
dpi’s picture

@Chernous_dn patch does not fix admin previews. That issue existed before the regression and seems unrelated. See #46 1:1

Chernous_dn’s picture

@dpi I'm talking about the patch it works for page, edit views and ajax rebuild views.

dpi’s picture

I understand it works, but I would like to get a Views maintainer to take a look at the issue to see if it is the correct resolution.

I would like to know why adding the CSS into Element/View::getInfo() is not a better fix, and why that also does not work.

#27 would not be a final fix as you would have to remove existing code from ViewExecutable.

Chernous_dn’s picture

@dpi I understand, thanks for the explanation.

dawehner’s picture

Mh did anyone tried to research why #38 fails for ajax? If the proper defaults are merged it, it sounds like a good change.

davidhernandez’s picture

See #42.

dawehner’s picture

See #42.

Well right, but why does that break the admin preview, according to @dpi on IRC.

dpi’s picture

Turns out AJAX test is getting a different response to what my browser receives.

Firefox views preview

[
  {
    "command": "settings",
    "settings": {....},
    "merge": true
  },
  {
    "command": "insert",
    "method": "append",
    "selector": "body",
    "data": ....,
    "settings": null
  },
]

ViewAjaxTest JSON response

[
  {
    "command": "settings",
    "settings": {....},
    "merge": true
  },
  {
    "command": "add_css",
    "data": ....
  },
  {
    "command": "insert",
    "method": "replaceWith",
    "selector": ".js-view-dom-id-",
    "data": ....
    "settings": null
  }
]

Thats why tests are OK in head I guess

davidhernandez’s picture

Well right, but why does that break the admin preview, according to @dpi on IRC.

Dunno, but I can confirm the preview is not working. I guess the construct in ViewExecutable was a better place to put it initially, and that's why it was there. Does the ajax request then not hit any pre-rendering?

olli’s picture

$executable->destroy(); in ViewUI::renderPreview() seems to reset $executable->element and lose the attached default library that is set in __construct, see #16.

olli’s picture

FileSize
1.32 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,444 pass(es), 4 fail(s), and 4 exception(s). View
808 bytes

Would this work?

Status: Needs review » Needs work

The last submitted patch, 57: 2529748-57.patch, failed testing.

davidhernandez’s picture

Status: Needs work » Needs review
FileSize
2.36 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,503 pass(es). View
1.04 KB

Not sure of the fix, but it works. The CSS is there for the page and preview. Obviously, attaching the library for all Views will make it work, I'm just not entirely clear of the original intention.

The add_css would not be in the response because the CSS is already there, so I removed that from the test. Makes sense?

mpotter’s picture

What's the status of this one? Is the patch in #59 the current recommended solution? #59 works for me on both the admin preview and main page showing the view. But don't know enough about the other issues in this thread to mark it RTBC yet.

webchick’s picture

Looks like we're still missing some test coverage here.

jhedstrom’s picture

Issue tags: -Needs tests
FileSize
586 bytes
586 bytes
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,002 pass(es), 1 fail(s), and 0 exception(s). View
2.94 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,968 pass(es). View

It should be ok to add the CSS on all views (views.module.css is quite small).

This adds a test to check for that file on grid views (I picked the grid test since that prompted this issue initially).

The last submitted patch, 62: 2529748-62-TEST-ONLY.patch, failed testing.

The last submitted patch, 62: 2529748-62-TEST-ONLY.patch, failed testing.

davidhernandez’s picture

We have test coverage now. Any takers to review?

pixelite’s picture

Status: Needs review » Reviewed & tested by the community

I applied the patch in #62 and the grid CSS now works in preview and in the Bartik theme.

pixelite’s picture

Before patch in #62

After patch in #62

espurnes’s picture

patch #62 works perfect. Thank you jhedstrom

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 3d0a938 and pushed to 8.0.x. Thanks!

  • alexpott committed 3d0a938 on
    Issue #2529748 by Chernous_dn, jhedstrom, cilefen, davidhernandez, olli...

Status: Fixed » Closed (fixed)

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