Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[4.1] Images in the Smart Search results #32450

Closed
wants to merge 35 commits into from

Conversation

sakiss
Copy link
Contributor

@sakiss sakiss commented Feb 17, 2021

Summary of Changes

The PR allows the use of images in the finder's results.

A new setting was added named "Result Image" both in the com_finder's (aka Smart Search) configuration and it's respective "search" menu item.

Testing Instructions

  1. Go to the "Smart Search" component, press the "Clear Index" button and then the "Index".
  2. Press the "Options" button to load the configuration and enable the "Result Image" setting.
  3. Use the smart search in the site's front-end and check the returned articles that have "Intro Image" set.

Actual result BEFORE applying this Pull Request

Smart_Search_1

Expected result AFTER applying this Pull Request

Smart_Search2

Sidenotes

Atm only the com_content finder plugin uses the images API function. Support for the rest plugins will be added in a next PR.
No styling is added, though a new setting (Image class) is introduced, according to the setting of com_content's config.
If anyone thinks that additional styling should be applied, plz commit.

Documentation Changes Required

DKN

@joomla-cms-bot joomla-cms-bot added Language Change This is for Translators PR-4.0-dev labels Feb 17, 2021
@sakiss sakiss changed the title Images in the Smart Search results [4.0] Images in the Smart Search results Feb 17, 2021
@richard67
Copy link
Member

Code style still bad, using spaces for indentation, see here https://ci.joomla.org/joomla/joomla-cms/40181/1/6 .

Another thing is that 4.0 is already in Beta phase so we have a feature freeze, and this here is a new feature.

So this PR should be made for the 4.1-dev branch.

@ceford
Copy link
Contributor

ceford commented Feb 18, 2021

I see the Intro image in the results list but in one instance followed by an error panel starting:

( ! ) Notice: Undefined variable: extraAttr in /Users/ceford/Sites/joomla-cms-4/components/com_finder/tmpl/search/default_result.php on line 96


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/32450.

@brianteeman
Copy link
Contributor

Just to be 100% clear. The decision was not to remove all descriptions. The decision was to display descriptions on screen instead of as a popup and only to include descriptions that it was felt were essential.

@richard67
Copy link
Member

@brianteeman Thanks for clarifying and explaining it more precise than I did. It's exactly like you say.

@richard67
Copy link
Member

Now the problem remains that if this is a new feature then it should go into 4.1.

@zero-24 What's your opinion?

@sakiss
Copy link
Contributor Author

sakiss commented Feb 18, 2021

Just to be 100% clear. The decision was not to remove all descriptions. The decision was to display descriptions on screen instead of as a popup and only to include descriptions that it was felt were essential.

Thanks for clarifying @brianteeman.
Then the addition of a description in a "vague" field name abides with the taken decision. Right?
Of course the answer is subjective to how someone perceives something as vague. But this by no means can be taken as inconsistency.

@zero-24
Copy link
Contributor

zero-24 commented Feb 18, 2021

Now the problem remains that if this is a new feature then it should go into 4.1.

@zero-24 What's your opinion?

That question should be directed at @bembelimen but i personally would agree to move this to 4.1

@chmst
Copy link
Contributor

chmst commented Feb 18, 2021

Everyting what is a nice idea and improvement, but does not fix a bug should be 4.1. as we did for example with my pr #32223

@N6REJ N6REJ added this to the Joomla 4.1 milestone Feb 18, 2021
@universewrld
Copy link

@sakiss thanks for your code! I originally wrote about this idea #27815, but it was closed.

@humblehumanbeing
Copy link

Sometimes we need to use full article image instead of intro article image. I think this pr should check for full article image also if intro images does not exist.

@ceford
Copy link
Contributor

ceford commented Feb 20, 2021

I have tested this item ✅ successfully on 50afef0


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/32450.

@Quy Quy removed the PR-4.0-dev label Feb 23, 2021
@Fedik
Copy link
Member

Fedik commented Sep 1, 2021

please also remove properties, it already handled by __get/__set methods,
see my previous comment

@sakiss
Copy link
Contributor Author

sakiss commented Sep 1, 2021

@bembelimen Unfortunately i am not aware of BS5 and the classes that J4 uses in general. I would appreciate it, if someone more knowledgable on that, have a look. Btw. The image uses the "Image Class" property/setting, which was introduced for the joomla content as well (not sure if this is still valid) and is supposed to provide the styling to the image.

@Quy Quy removed the RTC This Pull Request is Ready To Commit label Sep 1, 2021
@Fedik
Copy link
Member

Fedik commented Sep 1, 2021

You mean to get/set the image properties using the __get/__set functions?

Correct, please read the comment for the method:

/**
* The magic set method is used to push additional values into the elements
* array in order to preserve the cleanliness of the object.
*
* @param string $name The name of the element.
* @param mixed $value The value of the element.
*
* @return void
*
* @since 2.5
*/
public function __set($name, $value)
{
$this->setElement($name, $value);
}

Then we will have consistency. This allows us to add any extra value to the result object, without introducing extra properties and getters/setters, for each.

@@ -48,6 +49,22 @@
$description = HTMLHelper::_('string.truncate', StringHelper::substr($full_description, $start), $desc_length, true);
}

$showImage = $this->params->get('show_image', 0);

if ($showImage && !empty($this->result->getElement('imageUrl')))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you do not need use empty check, just do:

if ($showImage && $this->result->imageUrl)

Remember we have a __get

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$this->result->imageUrl and $this->result->imageAlt no longer exist, based on the last commit.

The imageUrl and imageAlt are now part of the Joomla\Component\Finder\Administrator\Indexer\Result::$elements
which can only be accessed through the __get() or the getElement() functions.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, do you know what __get()/__set() doing? :)
Please check how the code access to $item->title and other values.

$this->result->imageUrl and $this->result->imageAlt no longer exist

Correct, they not need to exist. That is magic of magic methods.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please test your suggestions before writing them down.
$this->result->imageUrl and $this->result->imageAlt are null since they do not exist.

If you want to provide a working example based on that suggestion, please commit.

Copy link
Member

@Fedik Fedik Sep 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is null only when it empty,
if you set a value while indexing then it will be not null here

if (!empty($images) && $images->image_intro)
{
$item->setElement('imageUrl', $images->image_intro);
$item->setElement('imageAlt', $images->image_intro_alt ?? '');
Copy link
Member

@Fedik Fedik Sep 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

look how $item->title is set, and do the same:

$item->imageUrl = $images->image_intro;
$item->imageAlt = $images->image_intro_alt ?? '';

Copy link
Contributor Author

@sakiss sakiss Sep 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is exactly how it was before pointing me out to use the getters/setters. Probably you are commenting without checking my commits.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, here it was correct before,

before pointing me out to use the getters/setters

It was about Result class

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry i cannot go back and forth. I am closing that as resolved since it works and it's indeed much better than setting public properties. For any further change please commit.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well, title was a bad example sorry, however please do:

$item->imageUrl = $images->image_intro;
$item->imageAlt = $images->image_intro_alt ?? '';

No need to create any properties, just do this.

Copy link
Contributor Author

@sakiss sakiss Sep 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Obviously you did not bother to examine the finder's code, neither to test what you are writing.
Since you are wasting my time and i have no appetite to talk with toddlers, i am closing that.

@sakiss sakiss closed this Sep 1, 2021
@richard67
Copy link
Member

@sakiss You've closed this by purpose or by accident?

@sakiss
Copy link
Contributor Author

sakiss commented Sep 1, 2021

@richard67 on purpose for obvious reasons

@sakiss sakiss deleted the finder_images branch September 1, 2021 18:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Language Change This is for Translators Updates Requested Indicates that this pull request needs an update from the author and should not be tested.
Projects
None yet
Development

Successfully merging this pull request may close these issues.