Support from Acquia helps fund testing for Drupal Acquia logo

Comments

star-szr’s picture

Issue tags: +Needs manual testing

Tagging.

joelpittet’s picture

Status: Active » Needs review
FileSize
3.15 KB

split from meta

thedavidmeister’s picture

Status: Needs review » Needs work

Review for #2:

+ * - content: The content for this seciton preview.

"section"

That's all that I can see for coding style.

This issue needs manual testing steps in the issue summary and for somebody to follow the steps and test the markup.

star-szr’s picture

Issue tags: +Novice

Creating a revised patch and interdiff based on #3 would be a good novice task, tagging.

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!

thedavidmeister’s picture

Manual testing is also a good novice task!

joelpittet’s picture

Status: Needs work » Needs review
Issue tags: -Novice
FileSize
3.2 KB
1.37 KB

Cleanup from #3 thanks @thedavidmeister and @Cottser

Ready for manual testing.

kbasarab’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
43.08 KB
  1. Added new content to site, edited front page view and inspected view preview to ensure old code was applied.
  2. Applied patch from #6. Patch applied cleanly
  3. Cleared caches
  4. Saved theme page to ensure theme registry is cleared
  5. Edited frontpage view again and confirmed that correct HTML was being applied
  6. To verify added a class to preview-section in the TWIG template.
  7. Cleared caches and Theme registry again
  8. Confirmed additional class was added in preview (Screenshot attached)

Manual testing done as a group at Florida Drupalcamp 2013 core mentoring code sprint.

thedavidmeister’s picture

Issue tags: -Needs manual testing
dawehner’s picture

Status: Reviewed & tested by the community » Needs review

Not sure whether we want to commit a patch with a @todo in there :)

thedavidmeister’s picture

Issue tags: +@todo clean-up

Lots of the current patches have @todo for documentation of cryptic variables, especially those in the Views conversion.

thedavidmeister’s picture

Status: Needs review » Reviewed & tested by the community
dawehner’s picture

Ah I see, there is an explicit tag for @todo cleanups.

joelpittet’s picture

@dawehner if you know, or anybody from VDC knows just one of those @todo pieces, please let us know and we will plop them in there.

dawehner’s picture

+++ b/core/modules/views/views_ui/views_ui.theme.incundefined
@@ -415,7 +415,15 @@ function theme_views_ui_style_plugin_table($variables) {
+ *   - section: @todo.

The section which is rendered (for example title, rows or pager).

+++ b/core/modules/views/views_ui/views_ui.theme.incundefined
@@ -415,7 +415,15 @@ function theme_views_ui_style_plugin_table($variables) {
+ *   - title: @todo.

The human readable title of the section.

thedavidmeister’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Novice
joelpittet’s picture

Status: Needs work » Needs review
FileSize
1.41 KB
555.49 KB

Thanks @dawehner! Rolled them in from #14

Realized we don't need 'title', so moved the wording over to the twig file and removed that param documentation because it wasn't really a param. Tweaked the wording a bit on the first one too, I'm sure it will be changed again but hopefully that helps.

joelpittet’s picture

Issue tags: -Novice, -@todo clean-up

remove tags

thedavidmeister’s picture

Status: Needs review » Needs work

#16 - woooah, @joelpittet looks like you didn't pull latest 8.x before you started work. There's like 500kb of regression in that last patch. Can we please get a re-roll?

joelpittet’s picture

Status: Needs work » Needs review
FileSize
3.24 KB
1.41 KB

Whoops, thanks @thedavidmeister. Forgot to rebase against origin/8.x

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Great!

Fabianx’s picture

Issue tags: -Twig, -VDC

Status: Reviewed & tested by the community » Needs work
Issue tags: +Twig, +VDC

The last submitted patch, 1963988-19-twig-views-ui-view-preview-section.patch, failed testing.

star-szr’s picture

Issue tags: +Novice, +Needs reroll

Tagging for reroll, the Views UI module got moved to core/modules in #1820414: CHANGE NOTICE: Move views_ui.module directly into /core/modules/.

chrisjlee’s picture

Status: Needs work » Needs review
FileSize
3.18 KB
1.71 KB

reroll plus change applied as per #23.

dlu’s picture

Assigned: Unassigned » dlu

Working on it in PDX.

Gaelan’s picture

Assigned: dlu » Unassigned
Issue tags: -Needs reroll

@dlu Already rerolled. :)

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Novice

I think this is ready to fly. It was RTBCed by dawehner already, has been re-rolled, I re-reviewed the code and it looks good!

=> RTBC again, lets get this in.

Dries’s picture

Sadly, this patch no longer applies:

megatron:drupal dries$ git apply ../f.patch --index
error: patch failed: core/modules/views_ui/views_ui.theme.inc:474
error: core/modules/views_ui/views_ui.theme.inc: patch does not apply

I'm going to ask for a re-test to make sure.

Dries’s picture

Issue tags: -Twig, -VDC

#24: 1963988-reroll-patch19-25.patch queued for re-testing.

Status: Reviewed & tested by the community » Needs work
Issue tags: +Twig, +VDC

The last submitted patch, 1963988-reroll-patch19-25.patch, failed testing.

star-szr’s picture

Issue tags: +Novice, +Needs reroll

Okay now it does need a reroll. Thanks @Dries!

Gaelan’s picture

Assigned: Unassigned » Gaelan

Rerolling.

Gaelan’s picture

Assigned: Gaelan » Unassigned
Status: Needs work » Needs review
Issue tags: -Novice, -Needs reroll
FileSize
3.25 KB

Rerolled.

Gaelan’s picture

EDIT: Whoops, double post.

Gaelan’s picture

Assigned: Unassigned » Gaelan
Status: Needs review » Needs work

I think I messed this up. About to try again.

Gaelan’s picture

Assigned: Gaelan » Unassigned
Status: Needs work » Needs review
Issue tags: -Twig, -VDC
FileSize
3.19 KB

Rererolled.

star-szr’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +Twig, +VDC

Thanks @Gaelan!

Back to RTBC per #27 as long as testbot agrees.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

For reasons I can not explain... I can not download the patch in #33 I get a page not found :( http://drupal.org/files/views_ui.1963988.33.theme_preview_section_twig_0...

Any chance of uploading again?

drupalninja99’s picture

I also get a page not found

joelpittet’s picture

Status: Needs work » Needs review
FileSize
3.19 KB

Re-roll from #24 the only difference is there from #33 that I guess is what @Gaelan was intending to post in #36 was the removal of the preprocess function.

Status: Needs review » Needs work
Issue tags: -Twig, -VDC

The last submitted patch, 1963988-41-twig-views-ui-view-preview-section.patch, failed testing.

drupalninja99’s picture

Status: Needs work » Needs review
Issue tags: +Twig, +VDC
drupalninja99’s picture

I ran the test that failed locally with no errors, going to try again.

dawehner’s picture

Status: Needs review » Needs work
+++ b/core/modules/views_ui/templates/views-ui-view-preview-section.html.twigundefined
@@ -0,0 +1,21 @@
+ * @see template_preprocess()

Afaik we remove all of these now.

jenlampton’s picture

Issue tags: +Novice

tagging

joelpittet’s picture

Status: Needs work » Needs review
Issue tags: -Novice
FileSize
3.16 KB
1.69 KB

Cleanup docs from #45 and V to v.

dawehner’s picture

+++ b/core/modules/views_ui/templates/views-ui-view-preview-section.html.twigundefined
@@ -0,0 +1,20 @@
+ * Default theme implementation for a iews UI preview section.

... missing v in "views" :)

joelpittet’s picture

Whoops, well maybe we should call them `iews` from now on:P Then it won't be confused with the concept of a view and view modes:P

dawehner’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +IDC

Ha!

thedavidmeister’s picture

@joelpittet - I vote they should be called Iews (capital I) ;)

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 7d267ff and pushed to 8.x. Thanks!

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

Anonymous’s picture

Issue summary: View changes

remove sandbox link