Currently, panopoly_magic builds the HTML markup for previews directly in some hook_preprocess_X() functions. This means that the theme can't change the markup used for the previews!

Radix works around this by copying the code for those hook_preprocess_X() functions and then changing the markup, but this means that Radix can break when we change how panopoly_magic works (for example, #2155377: Only show one widget preview at a time when adding content in panels).

So, we should move the generation of the markup for the previews into a theme function/template!

Screenshot of Radix

TODO

  1. Audit markup and class names
  2. Make sure this will allow Radix to customize the markup how it wants: #2502939: Incompatible with Panopoly's "Single" preview mode on the "Add content" modal
  3. Write tests for style plugin preview
  4. Write tests for Stylizer preview: deferred to new issue #2507609: Write tests for live preview of custom Stylizer styles
CommentFileSizeAuthor
#11 panopoly_test-theme-preview-2496027-11.patch12.06 KBdsnopek
#9 panopoly_test-theme-preview-2496027-9.patch2.18 KBdsnopek
#9 interdiff.txt7.9 KBdsnopek
#9 panopoly_magic-theme-preview-2496027-9.patch15.86 KBdsnopek
#7 panopoly_test-theme-preview-2496027-7.patch1.63 KBdsnopek
#7 interdiff.txt5.95 KBdsnopek
#7 panopoly_magic-theme-preview-2496027-7.patch11.56 KBdsnopek
#5 Selection_090.png60.41 KBdsnopek
#4 panopoly_magic-theme-preview-2496027-4.patch6.83 KBdsnopek

Comments

dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
commented
Issue tags: +sprint
gusaus’s picture

gusaus commented

I was going to post an issue, but it looks like #2155377: Only show one widget preview at a time when adding content in panels might be the related to why previews on this radix subtheme now look like this?
http://monosnap.com/image/X7BPU3lm0mdrTaVKyUOGivpF6INlPe

dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
commented

Yes. Until Radix is updated (ideally with this issue fixed!) you should use the old "Automatic" preview mode with Radix-based themes.

dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
commented
Status: Active » Needs review
Related issues: +#2467073: Preview fieldset overflows in Firefox if using a very large image
StatusFileSize
new panopoly_magic-theme-preview-2496027-4.patch6.83 KB

Here's a quick attempt at this! It tries to keep the markup exactly the same, although I think we should try to re-evaluate it, especially some of the class names.

Linking to #2467073: Preview fieldset overflows in Firefox if using a very large image which is about switching the preview away from using <fieldset> altogether! That'll be much easier to implement once this one is done.

EDIT: Here is a build on Travis-CI: https://travis-ci.org/panopoly/panopoly/builds/65994673

dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
commented
Issue summary: View changes
StatusFileSize
new Selection_090.png60.41 KB

The tests on Travis-CI pass!

I added a screenshot and a TODO list to the issue summary. Namely, we need to make sure this will work with Radix and add tests for any of the code we are changing which doesn't already have tests.

And I created a campanion issue in the Radix queue for using the changes on this issue once they're done:

#2502939: Incompatible with Panopoly's "Single" preview mode on the "Add content" modal

lsolesen’s picture

lsolesen commented 
$ git apply panopoly_magic-theme-preview-2496027-4.patch                                                
panopoly_magic-theme-preview-2496027-4.patch:120: trailing whitespace.                                                                  
/**                                                                 
warning: 1 line adds whitespace errors.
dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
commented
Issue summary: View changes
Related issues: +#2502939: Incompatible with Panopoly's "Single" preview mode on the "Add content" modal
StatusFileSize
new panopoly_magic-theme-preview-2496027-7.patch11.56 KB
new interdiff.txt5.95 KB
new panopoly_test-theme-preview-2496027-7.patch1.63 KB

Here's a new version of this patch which changes some of the class names used in the markup to be more specific to panopoly_magic. This requires some small changes to the tests. Anyway, this crosses TODO #1 off the list.

Also, I was able to show in #2502939: Incompatible with Panopoly's "Single" preview mode on the "Add content" modal that this will work for Radix, so I'm also crossing off TODO #2!

All that remains is adding some test coverage for some things this patch affects which weren't previously tested.

EDIT: Here's a Travis build for the latest patches: https://travis-ci.org/panopoly/panopoly/builds/67040765

dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
commented
Status: Needs review » Needs work

Hrm. Another thing just occurred to me: should we be using the 'class_array' and 'classes' variables in the template, rather than setting 'class' on 'attributes'? I think that makes sense to be consistent with the rest of Drupal.

Also we should probably use classes/attributes on the 'panopoly_magic_preview_link' theme function as well.

Marking as "Needs work"!

dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
commented
Status: Needs work » Needs review
StatusFileSize
new panopoly_magic-theme-preview-2496027-9.patch15.86 KB
new interdiff.txt7.9 KB
new panopoly_test-theme-preview-2496027-9.patch2.18 KB

Ok, here's another set of patches that represents an even bigger refactor in markup and class names. In testing with Radix, I discovered that it's unfortunately necessary to keep the old class names around so we don't break anything. A new version of Radix could, of course, fix this, but our releases aren't synchronized and if it's a problem in Radix, it's probably a problem in other themes too!

Also, I made use of the 'classes_array' variable like I mentioned in #8.

So, now for real, all we need to do is add the missing test coverage. :-)

EDIT: Here is a Travis build of the latest patches: https://travis-ci.org/panopoly/panopoly/builds/67058477

dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
commented

Both the Travis builds from #9 and #7 have passed!

dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
commented
Issue summary: View changes
Related issues: +#2507609: Write tests for live preview of custom Stylizer styles
StatusFileSize
new panopoly_test-theme-preview-2496027-11.patch12.06 KB

Here is a new patch for panopoly_test which adds tests for live preview in the style plugin! This crosses off TODO #3.

I manually tested that this works with stylizer, but I think I'm going to defer TODO #4 to a new issue: #2507609: Write tests for live preview of custom Stylizer styles

We're not currently setup to test stylizer (we'd need to enable it when setting up for the tests) and it's not that commonly used. I don't want to hold up something we need to support Radix, which is very commonly used, in order to provide tests for something that's rarely used.

If this passes the automated tests on Travis-CI, I'll commit it! But any manual testing/review is very welcome. :-)

EDIT: Here's the Travis build: https://travis-ci.org/panopoly/panopoly/builds/67215147

dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
commented
Status: Needs review » Fixed

Tests passed! I'm going to commit this so that we're sure the next release will allow us to fix this in Radix. Test still (always!) welcome. :-)

  • dsnopek committed ca87e8d on 7.x-1.x 
    Update Panopoly Magic and Test for Issue #2496027 by dsnopek: Generate...

Status: Fixed » Closed (fixed)

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