Support from Acquia helps fund testing for Drupal Acquia logo

Comments

caschbre’s picture

We could potentially just use a text or link field and then theme it so the image renders with the link. Using linkit would be ideal but requires an upgrade.

Related: #2186541: Upgrade linkit module 3.1+

lsolesen’s picture

dsnopek’s picture

I agree this would be a useful feature! I had a client ask for it, but it wasn't high enough priority to get implemented as part of that project. :-/

caschbre’s picture

Note: a patch needs review at #2186541: Upgrade linkit module 3.1+ that can open up the way to get this issue resolved.

dsnopek’s picture

Status: Active » Needs review
FileSize
10.53 KB

Attached is the start of a patch for this! It includes a linkit profile for use with fields, but tries to keep the dependency on linkit optional. I'm not entirely sure that panopoly_widgets is the best place for this profile, but I haven't had an enough time to think it through.

In any case, this needs:

  1. More testing, especially with (a) a fresh install and (b) to make sure that linkit is really optional in a child distribution that doesn't use panopoly_wysiwyg
  2. Ideally, some automated tests
dsnopek’s picture

Here is a new patch - there are some minor changes to functionality and some tests. I've been having some trouble running the tests locally so I'm going to try then on Travis-CI in a moment.

dsnopek’s picture

FileSize
2.76 KB

Didn't get the tests quite right - here's another attempt.

dsnopek’s picture

FileSize
2.78 KB

Closer! Here's another attempt.

dsnopek’s picture

FileSize
2.77 KB

Hopefully, the final attempt!

  • dsnopek committed 4792caa on 7.x-1.x
    Update Panopoly Widgets and Test for Issue #2013965 by dsnopek: ability...
dsnopek’s picture

Status: Needs review » Fixed

Ok! The test is finally working. :-) Committed!

dsnopek’s picture

  • dsnopek committed f389395 on 7.x-1.x
    Revert Panopoly Widgets and Test for Issue #2013965 by dsnopek: ability...
dsnopek’s picture

Status: Fixed » Needs work

Due to the issues with the upgrade tests, I've decided to actually revert this and fix it before committing again. I really don't want to leave the tests broken for a prolonged period of time.

That means I'm also closing #2382495: Image widget with Linkit tests fail after some upgrades as a duplicate.

And here is the description from it:

In these upgrade tests on Travis-CI (but not all):

  1. 1.11
  2. 1.10
  3. 1.9
  4. 1.8
  5. 1.6
  6. 1.1
  7. 1.0-rc5
  8. 1.0-rc4

... the image widget with Linkit tests fail!

I suspect it has to do with the order of hook_update_N() functions. I think that panopoly_widgets_update_7010() needs to run after linkit_update_7301().

See this Travis-CI build:

https://travis-ci.org/panopoly/panopoly/builds/42113770

dsnopek’s picture

Status: Needs work » Needs review
FileSize
11.61 KB

Here is a new patch that attempts to fix the upgrade issues!

dsnopek’s picture

Status: Needs review » Needs work
FileSize
9.55 KB
12.11 KB
2.77 KB

Here's a new set of patches that fix the upgrade tests! I also make some minor tweaks to texts and the order of the fields.

However, visual layout of the "Link" field is kind of insane and I'd like to rearrange it before committing again:

I'd prefer to see:

  1. The button to the right of the text field
  2. The description directly below the text field (ie. above the checkbox)

Update: Here's the current Travis-CI run with these patches: https://travis-ci.org/dsnopek/panopoly/builds/42233548

  • dsnopek committed 4792caa on 8.x-1.x
    Update Panopoly Widgets and Test for Issue #2013965 by dsnopek: ability...
  • dsnopek committed f389395 on 8.x-1.x
    Revert Panopoly Widgets and Test for Issue #2013965 by dsnopek: ability...

  • dsnopek committed 4792caa on 8.x-2.x
    Update Panopoly Widgets and Test for Issue #2013965 by dsnopek: ability...
  • dsnopek committed f389395 on 8.x-2.x
    Revert Panopoly Widgets and Test for Issue #2013965 by dsnopek: ability...
dsnopek’s picture

Status: Needs work » Needs review
FileSize
13.03 KB
2.45 KB

Here's a re-roll for the latest version of Panopoly and it removes some options/features which I think addresses my UX concerns in #16. If this passes tests, I'm going to commit it again - it's long overdue. :-)

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

EDIT-2: That one was broken by an unrelated issue, here's a new one: https://travis-ci.org/panopoly/panopoly/builds/146666491

dsnopek’s picture

FileSize
13.08 KB
444 bytes

Tests failed because the Feature was overridden. Here's an attempt to fix that!

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

  • dsnopek committed d8952e3 on 7.x-1.x
    Update Panopoly Widgets and Test for Issue #2013965 by dsnopek, caschbre...
dsnopek’s picture

Tests passed, so committed!

Here's a couple of follow-up issues that depended on this issue:

dsnopek’s picture

Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

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

dsnopek’s picture

Version: 7.x-1.x-dev » 8.x-2.x-dev
Status: Closed (fixed) » Patch (to be ported)

This feature needs to be ported to Drupal 8! I think this got lost in the shuffle because the Drupal 8 version of the Image widget was created before this was committed, then this was committed, and now Drupal 8 doesn't have this feature

dsnopek’s picture

mfrausto’s picture

Here my first attempt to port this feature to panopoly 8

dsnopek’s picture

Status: Patch (to be ported) » Needs work

Thans!

+++ profiles/panopoly/modules/panopoly/panopoly_widgets/panopoly_widgets.module
@@ -51,3 +55,37 @@ function panopoly_widgets_configure_form_submit($form, &$form_state) {
+    $img = '<img src="' . $image_path . '" />';
+    $rendered_image = render($img);
+    $image_markup = Markup::create($rendered_image);
+    $variables["content"]["field_panopoly_widgets_link"][0]["#title"] = $image_markup;

Hm. I don't like that this is rendering in a preprocess hook. In D8, it's best to render as late as possible, ideally, just creating render arrays in PHP code, and only rendering in templates.

What type of element is used for 'field_panopoly_widget_image'? Perhaps it allows for a way to add a link?

mfrausto’s picture

@dsnopek Sure, Changed a little bit the way is rendering using render arrays...

I see 'field_panopoly_widget_image' is image type, Not sure if it allows to add a link, I'll research and try

dsnopek’s picture

The latest patch is better, but still not ideal. If there was a way to just put the URL on part of the existing render array in such a way that the image would get rendered wrapped in a link, that'd be the best outcome. But I guess that's what you're researching!

I don't know if these parts of the patch will remain after your research, but here's a couple quick comments:

  1. +++ profiles/panopoly/modules/panopoly/panopoly_widgets/panopoly_widgets.module
    @@ -5,11 +5,15 @@
    +define('PANOPOLY_IMAGE_BUNDLE', 'panopoly_widgets_image');
    

    This constant should instead be 'PANOPOLY_WIDGETS_IMAGE_BUNDLE' to namespace it for this module

  2. +++ profiles/panopoly/modules/panopoly/panopoly_widgets/panopoly_widgets.module
    @@ -51,3 +55,39 @@ function panopoly_widgets_configure_form_submit($form, &$form_state) {
    +function get_field_panopoly_image_url($field = NULL) {
    

    This function should be prefixed with 'panopoly_widgets_' to namespace it to this module. Given that it's an internal utility function, I might put a leading underscore on there too. So, like '_panopoly_widgets_image_get_url_field()' or something like that

mfrausto’s picture

This is my attempt to improve the patch, also added update hook.

mfrausto’s picture

Status: Needs work » Needs review

  • dsnopek committed 178721d on 8.x-2.x
    Update Panopoly Widgets for Issue #2013965 by dsnopek, mfrausto,...
dsnopek’s picture

Status: Needs review » Fixed

Thanks! Worked in my testing. :-) Committed with two minor changes:

  • Changed the description on the update hook
  • Hid the "Link Text" field from the form (it's unused)

  • dsnopek committed 8237466 on 7.x-1.x
    Issue #2013965 by dsnopek: ability to link images placed with image...
  • dsnopek committed c025b8b on 7.x-1.x
    Revert "Issue #2013965 by dsnopek: ability to link images placed with...
  • dsnopek committed f67936c on 7.x-1.x
    Revert "Issue #2013965 by dsnopek: ability to link images placed with...
  • dsnopek committed fdce104 on 7.x-1.x
    Issue #2013965 by dsnopek: ability to link images placed with image...
  • dsnopek committed 2756a5c on 7.x-1.x
    Issue #2013965 by dsnopek, caschbre, lsolesen, Andrew_Mallis: ability to...
  • dsnopek committed a631e16 on 7.x-1.x
    Issue #2013965 by dsnopek, caschbre, lsolesen, Andrew_Mallis: ability to...

  • dsnopek committed f67936c on 8.x-2.x
    Revert "Issue #2013965 by dsnopek: ability to link images placed with...
  • dsnopek committed fdce104 on 8.x-2.x
    Issue #2013965 by dsnopek: ability to link images placed with image...
  • dsnopek committed 9c6f57d on 8.x-2.x authored by mfrausto
    Issue #2013965 by dsnopek, mfrausto, caschbre, lsolesen: ability to link...

  • dsnopek committed 8237466 on 8.x-2.x
    Issue #2013965 by dsnopek: ability to link images placed with image...
  • dsnopek committed c025b8b on 8.x-2.x
    Revert "Issue #2013965 by dsnopek: ability to link images placed with...
  • dsnopek committed 2756a5c on 8.x-2.x
    Issue #2013965 by dsnopek, caschbre, lsolesen, Andrew_Mallis: ability to...

Status: Fixed » Closed (fixed)

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