Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Comment | File | Size | Author |
---|---|---|---|
#59 | twig-themes-views-ui-container-1915026-59.patch | 2.89 KB | drupalninja99 |
#59 | interdiff.txt | 630 bytes | drupalninja99 |
#57 | twig-themes-views-ui-container-1915026-57.patch | 2.72 KB | rvilar |
#52 | twig-1915026-52.patch | 2.72 KB | eromero1 |
#48 | twig-1915026-48.patch | 2.71 KB | eromero1 |
Comments
Comment #1
steveoliver CreditAttribution: steveoliver commentedMoving to core queue.
Comment #3
steveoliver CreditAttribution: steveoliver commentedHmmm... someone wanna take a stab at this -- seems like it should be working.
Comment #4
steveoliver CreditAttribution: steveoliver commentedWhy you steal tags, bot?
Comment #5
steveoliver CreditAttribution: steveoliver commentedThis needs to be removed (is removed in updated patch). It was part of another patch.
Other than that, I don't see how this isn't passing tests...
Comment #6
steveoliver CreditAttribution: steveoliver commentedExpect it to fail, as it does locally. Still don't get why. Expect facepalm.
Comment #8
dawehnerLet's use "Preprocess variables for views-ui-container.html.twig" like template_preprocess_page/template_preprocess_node does it.
Comment #9
steveoliver CreditAttribution: steveoliver commentedRe: #8: I know 'prepares' is a departure from what we've done historically, but maybe we consider using 'prepares variables' instead of 'preprocesses' to help clarify what it is we're doing in these functions? See #1825464: [meta] Documentation blocks for preprocess functions and #1913208: [policy] Standardize template preprocess function documentation.
Comment #10
damiankloip CreditAttribution: damiankloip commentedThe fails are from whitespace in the template somehow. So this should pass (changed the template). We should also wrap t() to assert the message text I think, as the markup text in the calling code is doing that.
Comment #11
dawehnerThis looks perfect beside of one small thing.
:(
Comment #12
damiankloip CreditAttribution: damiankloip commentedHa, I blame that on PHPStorm, sublime did that for me ;)
Comment #13
dawehnerYou can configure that :)
Comment #14
xjm#12: 1915026-12.patch queued for re-testing.
Comment #15
steveoliver CreditAttribution: steveoliver commentedI'd run with this, to keep inline with #1913208: [policy] Standardize template preprocess function documentation.
Comment #16
steveoliver CreditAttribution: steveoliver commentedComment #17
star-szrThis is looking great so far, thanks to everyone who's worked on the issue!
Minor documentation nitpick:
Should be something more like "Prepares variables for Views UI container templates". Also the $variables are not documented at all.
Comment #18
star-szrTagging.
Comment #19
thedavidmeister CreditAttribution: thedavidmeister commentedReview of #15:
We're not talking about PHP data types in Twig templates.
Preprocess needs an @param for the $variables array.
Comment #20
star-szrTagging as a novice task for the documentation tweaks in #17 and #19. Also, @thedavidmeister and myself are talking about the same $variables array in both our comments, see #1913208: [policy] Standardize template preprocess function documentation for an example preprocess docblock that has the variables documented.
Here's the reference for PHP data types: http://drupal.org/node/1823416#datatypes
If you'd like to work on this, please assign the issue to yourself. If you get stuck, drop by #drupal-twig on IRC or post your questions here. Thanks!
Comment #21
2ndmile CreditAttribution: 2ndmile commentedComment #22
2ndmile CreditAttribution: 2ndmile commentedNewb alert. Please review and let me know if I am in the ballpark implementing #17 and #19.
Comment #24
2ndmile CreditAttribution: 2ndmile commentedDisregard #22. Please review. Implementing #17 and #19.
Comment #26
2ndmile CreditAttribution: 2ndmile commentedTrying again.
Comment #28
star-szrDocs are looking much better here, thanks @2ndmile!
This is probably what is causing all the test failures because we only changed documentation between #15 and #26 from what I can see. And don't worry, it happens :)
I hate to say it after you've worked so hard on the preprocess docs @2ndmile but we might be able to lose the preprocess function by changing the hook_theme() definition to use 'variables' => array('children', 'attributes') instead of 'render element' (same function where the 'templates' key is being added). See #1939086-36: Convert theme_dropbutton_wrapper() to Twig.
Comment #29
dawehnerAfaik the core standard for "render element" templates is to use the same name: see theme_menu_tree, template_preprocess_menu_tree
Comment #30
joelpittet@Cottser could you explain #28 a bit further? Would making that change just work or is that an API change? @dawehner are you refering to #28?
Here is a re-roll from #15 including all of @2ndmile changes, though the patches didn't apply well so the interdiff is from #15 and manually grabbed the changes from those patches.
Comment #31
star-szr@joelpittet - thanks! Check the interdiff at http://drupal.org/node/1939086#comment-7294324, it should just work. This is only used in
#theme_wrappers
just like theme_dropbutton_wrapper() was. Not sure that it's an API change because it's called in exactly the same way - but maybe.@dawehner it sounds like following that convention (#29) the template variable would be 'container' or 'views_ui_container', but I don't think it would make sense to do that. Only the 'attributes' are used for printing the container itself and the contents are printed inside the container. If we wanted to change the variable name 'content' or 'contents' maybe but then we wouldn't be able to get rid of the preprocess function :)
CNW to try removing the preprocess function, this would be a good novice task. @2ndmile - unassigning for now, feel free to pick this back up if you have time :)
Comment #32
echeese CreditAttribution: echeese commentedRerolled the patch in #30 because it did not apply. I am working on a fix
Comment #34
echeese CreditAttribution: echeese commentedMoved the template to the correct folder, tests should pass now.
Comment #35
Hydra CreditAttribution: Hydra commentedThe path actualy changed of the views test, here is a new version
Comment #36
Hydra CreditAttribution: Hydra commentedOkay, manual testing was going well for the template file usage.
The test itself is dying when trying to run it but this might more be a VDC issue, then a Twig issue.
Comment #37
star-szrComment #38
joelpittetNeeds a re-roll before profiling.
Comment #39
dsdeiz CreditAttribution: dsdeiz commentedRe-rolling.
Comment #41
star-szr#39: 1915026-39-views-ui-container.patch queued for re-testing.
Comment #42
joelpittetThank you @dsdeiz. Next we need to confirm output.
Here's a bit of a cleanup by removing the preprocess and Views to views.
Comment #43
waynethayer CreditAttribution: waynethayer commentedTested manually from:
/admin/structure/views/view/user_admin_people
Wrapper div (div.views-display-top) is missing id and classes:
class="views-display-top clearfix" id="views-display-top"
Comment #44
waynethayer CreditAttribution: waynethayer commentedWorked on this with @Cottser and found the bug. Classes and id are now being added to wrapper div.
Before:
<div class="views-display-top clearfix" id="views-display-top"><div class="dropbutton-wrapper dropbutton-processed dropbutton-multiple"><div class="dropbutton-widget"><ul id="views-display-extra-actions" class="dropbutton dropbutton-icon-processed"><li class="edit-details odd first dropbutton-action"><a href="/admin/structure/views/nojs/edit-details/user_admin_people/page_1" class="views-ajax-link views-ajax-processed-processed">Edit view name/description</a></li><li class="dropbutton-toggle"><button type="button" role="button"><span class="dropbutton-arrow"><span class="visually-hidden">List additional actions</span></span></button></li><li class="analyze even dropbutton-action secondary-action"><a href="/admin/structure/views/nojs/analyze/user_admin_people/page_1" class="views-ajax-link views-ajax-processed-processed">Analyze view</a></li><li class="clone odd dropbutton-action secondary-action"><a href="/admin/structure/views/view/user_admin_people/clone">Clone view</a></li><li class="reorder even last dropbutton-action secondary-action"><a href="/admin/structure/views/nojs/reorder-displays/user_admin_people/page_1" class="views-ajax-link views-ajax-processed-processed">Reorder displays</a></li></ul></div></div><h2 class="visually-hidden">Secondary tabs</h2><ul id="views-display-menu-tabs" class="tabs secondary views-ui-render-add-view-button-processed-processed"><li class="active"><a href="/admin/structure/views/view/user_admin_people/edit/page_1">Page<span class="visually-hidden">(active tab)</span></a></li><li class="add"><a href="#"><span class="icon add"></span>Add</a><ul class="action-list" style="display:none;"><li class="last"><input class="add-display button form-submit" formnovalidate="formnovalidate" type="submit" id="edit-displays-top-add-display-attachment" name="op" value="Attachment"></li><li><input class="add-display button form-submit" formnovalidate="formnovalidate" type="submit" id="edit-displays-top-add-display-block" name="op" value="Block"></li><li><input class="add-display button form-submit" formnovalidate="formnovalidate" type="submit" id="edit-displays-top-add-display-embed" name="op" value="Embed"></li><li><input class="add-display button form-submit" formnovalidate="formnovalidate" type="submit" id="edit-displays-top-add-display-feed" name="op" value="Feed"></li><li class="first"><input class="add-display button form-submit" formnovalidate="formnovalidate" type="submit" id="edit-displays-top-add-display-page" name="op" value="Page"></li></ul></li></ul></div>
After:
<div class="views-display-top clearfix" id="views-display-top"><div class="dropbutton-wrapper dropbutton-processed dropbutton-multiple"><div class="dropbutton-widget"><ul id="views-display-extra-actions" class="dropbutton dropbutton-icon-processed"><li class="edit-details odd first dropbutton-action"><a href="/admin/structure/views/nojs/edit-details/user_admin_people/page_1" class="views-ajax-link views-ajax-processed-processed">Edit view name/description</a></li><li class="dropbutton-toggle"><button type="button" role="button"><span class="dropbutton-arrow"><span class="visually-hidden">List additional actions</span></span></button></li><li class="analyze even dropbutton-action secondary-action"><a href="/admin/structure/views/nojs/analyze/user_admin_people/page_1" class="views-ajax-link views-ajax-processed-processed">Analyze view</a></li><li class="clone odd dropbutton-action secondary-action"><a href="/admin/structure/views/view/user_admin_people/clone">Clone view</a></li><li class="reorder even last dropbutton-action secondary-action"><a href="/admin/structure/views/nojs/reorder-displays/user_admin_people/page_1" class="views-ajax-link views-ajax-processed-processed">Reorder displays</a></li></ul></div></div><h2 class="visually-hidden">Secondary tabs</h2><ul id="views-display-menu-tabs" class="tabs secondary views-ui-render-add-view-button-processed-processed"><li class="active"><a href="/admin/structure/views/view/user_admin_people/edit/page_1">Page<span class="visually-hidden">(active tab)</span></a></li><li class="add"><a href="#"><span class="icon add"></span>Add</a><ul class="action-list" style="display:none;"><li class="last"><input class="add-display button form-submit" formnovalidate="formnovalidate" type="submit" id="edit-displays-top-add-display-attachment" name="op" value="Attachment"></li><li><input class="add-display button form-submit" formnovalidate="formnovalidate" type="submit" id="edit-displays-top-add-display-block" name="op" value="Block"></li><li><input class="add-display button form-submit" formnovalidate="formnovalidate" type="submit" id="edit-displays-top-add-display-embed" name="op" value="Embed"></li><li><input class="add-display button form-submit" formnovalidate="formnovalidate" type="submit" id="edit-displays-top-add-display-feed" name="op" value="Feed"></li><li class="first"><input class="add-display button form-submit" formnovalidate="formnovalidate" type="submit" id="edit-displays-top-add-display-page" name="op" value="Page"></li></ul></li></ul></div>
Comment #45
joelpittetHere are some profiling results:
Scenario:
Stark, /admin/structure/views/view/user_admin_people/
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51c6814c41b47&...
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51c6814c41b47&...
RTBC as per #13 and the profiling results are good.
Comment #46
alexpottThis should use format_string() and not t()... t() should not be used in tests.
Comment #47
jenlamptontagging
Comment #48
eromero1 CreditAttribution: eromero1 commentedTook a shot at rerolling it. Hopefully it runs smoother than egyptian silk.
Comment #49
sbudker1 CreditAttribution: sbudker1 commentedPatch seemed to work! I did not fine any errors in my testing. I dont know if it was as smooth as silk though ;). Good Work!
Comment #50
sbudker1 CreditAttribution: sbudker1 commentedComment #51
star-szr#48 is identical to #44, we still need to change the t() to format_string() like @alexpott mentioned in #46 :)
Comment #52
eromero1 CreditAttribution: eromero1 commentedThis one should work
Comment #53
sbudker1 CreditAttribution: sbudker1 commenteddibs!
Comment #54
sbudker1 CreditAttribution: sbudker1 commentedPatch worked! Built a view and content showed up normally, I did not come across any problems.
Comment #55
sbudker1 CreditAttribution: sbudker1 commentedPatch worked! Built a view and content showed up normally, I did not come across any problems.
Comment #56
damiankloip CreditAttribution: damiankloip commentedSorry.. We should use String::format() and not format_string().
Comment #57
rvilarThis patch uses String::format instead of format_string
Comment #59
drupalninja99 CreditAttribution: drupalninja99 commentedIt looks like the tests were failing bc String::format didn't have this use statement:
I added this line and re-ran the failing test locally and it passed. Attaching the re-rolled patch.
Comment #60
azinoman CreditAttribution: azinoman commentedThis patch worked! Added a view and things looked fine! Changing to reviewed and tested by the community!
Comment #61
alexpottCommitted 637beb9 and pushed to 8.x. Thanks!
Comment #62.0
(not verified) CreditAttribution: commentedUpdated issue summary.