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

[mod_articles_news] Option Intro/Full Images of the article #20169

Merged
merged 14 commits into from May 28, 2018
Merged

[mod_articles_news] Option Intro/Full Images of the article #20169

merged 14 commits into from May 28, 2018

Conversation

carlitorweb
Copy link
Member

@carlitorweb carlitorweb commented Apr 14, 2018

Pull Request for Issue #19599 , #16495

Summary of Changes

Option to use or not, the intro image field or the full image field of the article

Testing Instructions

  1. Go to Extensions ->Modules and create a new Articles - Newsflash
  2. Put Show Images option, for any of the 3 options that list. Also, configure the module to display correctly in some position of the template
  3. Go to Content -> Articles, and use in one of the articles that the module will show, any of the fields "Intro image" or "Full article image"
  4. Go to the frontend, and check depending on the option that you put in the module, it shows or not, the image correctly

Expected result

The module shows or not, the fields "Intro image" or "Full article image" of the article correctly

Actual result

The current option only display images in the article context.

Documentation Changes Required

New module option added.

Option to use or not, the intro image field or the full image field of
the article
{
$item->imageSrc = htmlspecialchars($images->image_intro, ENT_COMPAT, 'UTF-8');
$item->imageAlt = htmlspecialchars($images->image_intro_alt, ENT_COMPAT, 'UTF-8');
if ($images->image_intro_caption)
Copy link
Contributor

Choose a reason for hiding this comment

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

Add blank line above.

{
$item->imageSrc = htmlspecialchars($images->image_fulltext, ENT_COMPAT, 'UTF-8');
$item->imageAlt = htmlspecialchars($images->image_fulltext_alt, ENT_COMPAT, 'UTF-8');
if ($images->image_intro_caption)
Copy link
Contributor

Choose a reason for hiding this comment

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

Add blank line above.

$item->imageCaption = htmlspecialchars($images->image_intro_caption, ENT_COMPAT, 'UTF-8');
}
}
else if ($params->get('image') == "full" && isset($images->image_fulltext) && !empty($images->image_fulltext))
Copy link
Contributor

@Quy Quy Apr 14, 2018

Choose a reason for hiding this comment

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

Change to elseif. Change " to '. Change == to ===.

{
$item->introtext = preg_replace('/<img[^>]*>/', '', $item->introtext);
$images = json_decode($item->images);
$item->imageSrc = "";
Copy link
Contributor

@Quy Quy Apr 14, 2018

Choose a reason for hiding this comment

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

Change "" to ''

@@ -119,9 +119,32 @@ public static function getList(&$params)

$item->introtext = JHtml::_('content.prepare', $item->introtext, '', 'mod_articles_news.content');

if (!$params->get('image'))
// Get the data of the image, if it is going to be shown
if ($params->get('image') != "none")
Copy link
Contributor

Choose a reason for hiding this comment

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

Change " to '

$item->imageSrc = "";
$item->imageAlt = "";
$item->imageCaption = "";
if ($params->get('image') == "intro" && isset($images->image_intro) && !empty($images->image_intro))
Copy link
Contributor

@Quy Quy Apr 14, 2018

Choose a reason for hiding this comment

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

Change " to '. Change == to ===.

{
$item->imageCaption = htmlspecialchars($images->image_fulltext_caption, ENT_COMPAT, 'UTF-8');
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Remove blank line.

label="MOD_ARTICLES_NEWS_FIELD_IMAGES_LABEL"
description="MOD_ARTICLES_NEWS_FIELD_IMAGES_DESC"
class="btn-group btn-group-yesno"
default="0"
default="1"
Copy link
Contributor

Choose a reason for hiding this comment

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

Why 1?

Copy link
Member Author

Choose a reason for hiding this comment

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

Was a mistake. Now is okay

@@ -30,6 +30,8 @@ MOD_ARTICLES_NEWS_FIELD_TRIGGEREVENTS_DESC="Triggers additional plugin events to
MOD_ARTICLES_NEWS_FIELD_TRIGGEREVENTS_LABEL="Trigger Plugin Events"
MOD_ARTICLES_NEWS_FIELD_SHOWINTROTEXT_DESC="Show or hide the article intro text."
MOD_ARTICLES_NEWS_FIELD_SHOWINTROTEXT_LABEL="Show Intro Text"
MOD_ARTICLES_NEWS_OPTION_INTROIMAGE="Intro image"
Copy link
Contributor

Choose a reason for hiding this comment

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

Change to Image

<figure class="newsflash-image">
<img src="<?php echo $item->imageSrc;?>" alt="<?php echo $item->imageAlt;?>">
<figcaption>
<?php echo $item->imageCaption;?>
Copy link
Contributor

Choose a reason for hiding this comment

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

Add space after ;

@@ -22,6 +22,15 @@
</<?php echo $item_heading; ?>>
<?php endif; ?>

<?php if ($params->get('image') != "none" && !empty($item->imageSrc)) : ?>
<figure class="newsflash-image">
<img src="<?php echo $item->imageSrc;?>" alt="<?php echo $item->imageAlt;?>">
Copy link
Contributor

Choose a reason for hiding this comment

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

Add space after ;

@@ -22,6 +22,15 @@
</<?php echo $item_heading; ?>>
<?php endif; ?>

<?php if ($params->get('image') != "none" && !empty($item->imageSrc)) : ?>
Copy link
Contributor

Choose a reason for hiding this comment

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

Change " to '

@carlitorweb
Copy link
Member Author

Is done, thank you @Quy

Fixed default attribute of the image field inside the xml file.
Added a condition in case the caption of the image is empty, for no show
unnecessarily the figcaption tag
@ghost
Copy link

ghost commented Apr 15, 2018

I have tested this item ✅ successfully on a3b2f08


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

@bubi-luka
Copy link

I have tested this item ✅ successfully on a3b2f08


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

@ghost
Copy link

ghost commented Apr 15, 2018

Ready to Commit after two successful tests.

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Apr 15, 2018
$item->imageSrc = '';
$item->imageAlt = '';
$item->imageCaption = '';
if ($params->get('image') === 'intro' && isset($images->image_intro) && !empty($images->image_intro))
Copy link
Contributor

Choose a reason for hiding this comment

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

Add blank line above.

<?php if ($params->get('image') != 'none' && !empty($item->imageSrc)) : ?>
<figure class="newsflash-image">
<img src="<?php echo $item->imageSrc; ?>" alt="<?php echo $item->imageAlt; ?>">
<?php if(!empty($item->imageCaption)) : ?>
Copy link
Contributor

Choose a reason for hiding this comment

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

Add space after if

<option value="0">JNO</option>
<option value="intro">MOD_ARTICLES_NEWS_OPTION_INTROIMAGE</option>
<option value="full">MOD_ARTICLES_NEWS_OPTION_FULLIMAGE</option>
<option value="null">JNO</option>
Copy link
Contributor

Choose a reason for hiding this comment

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

Change to none?

@carlitorweb
Copy link
Member Author

Thank you again @Quy . I need have more careful with this small things.

@ReLater
Copy link
Contributor

ReLater commented Apr 19, 2018

I have tested this item ✅ successfully on 68e2e54


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

@@ -7,8 +7,8 @@ MOD_ARTICLES_NEWS="Articles - Newsflash"
MOD_ARTICLES_NEWS_FIELD_FEATURED_DESC="Show or hide articles marked as featured."
MOD_ARTICLES_NEWS_FIELD_FEATURED_LABEL="Featured Articles"
MOD_ARTICLES_NEWS_FIELD_CATEGORY_DESC="Select Articles from a specific Category or a set of Categories. If no selection will show all categories as default."
MOD_ARTICLES_NEWS_FIELD_IMAGES_ARTICLE_DESC="Display article intro or full image."
MOD_ARTICLES_NEWS_FIELD_IMAGES_ARTICLE_LABEL="Show Intro-Full Image"
MOD_ARTICLES_NEWS_FIELD_IMAGES_ARTICLE_DESC="Display article, intro or full image."
Copy link
Contributor

Choose a reason for hiding this comment

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

@brianteeman help pls.
I don't think that the comma is correct here. The string should describe something like:
"Show the intro image or the full image that you selected in the article"
but not
"Show article image or the intro image or the full image"

(for the images inside article text we have another setting)

Copy link
Contributor

Choose a reason for hiding this comment

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

sorry I thought it was display article or intro or full
in this context the word article is not needed and as you can see fro my error it can be misleading
i dont think we ever refer to them as article images any way so
why not simply say
Display the intro or full image

Copy link
Member Author

Choose a reason for hiding this comment

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

I wrote the "article" word, trying not to confuse the one who reads it, with the other param. "Intro image" and "Full image", are well known, but include the "article" word, I think it locates better which fields we're talking about.

Copy link
Contributor

Choose a reason for hiding this comment

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

Somehow or other but the comma is wrong. I myself would prefer a description without "article" here. For me an "article" image (inside the editor text) is distinct from an "intro" image is distinct from a "full" image. The description would be clearer for me as a not native English speaker.

@carlitorweb
Copy link
Member Author

Ok done. Hope is better now.

@ReLater
Copy link
Contributor

ReLater commented Apr 24, 2018

I have tested this item ✅ successfully on 5efd862


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

1 similar comment
@Quy
Copy link
Contributor

Quy commented Apr 24, 2018

I have tested this item ✅ successfully on 5efd862


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

@Quy
Copy link
Contributor

Quy commented Apr 24, 2018

RTC


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Apr 24, 2018
@mbabker mbabker added this to the Joomla 3.9.0 milestone Apr 29, 2018
@mbabker mbabker changed the base branch from staging to 3.9-dev May 28, 2018 14:58
@mbabker mbabker modified the milestones: Joomla 3.10.0, Joomla 3.9.0 May 28, 2018
@mbabker mbabker merged commit a7571f4 into joomla:3.9-dev May 28, 2018
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label May 28, 2018
@carlitorweb carlitorweb deleted the mod_article_news_img_option branch June 20, 2018 12:13
@ute-sch
Copy link

ute-sch commented May 13, 2019

Please help (my Joomla's up to date):
The Article-Newsflash Module doesn't show the Intro or Full image, as offered in the field.

What I found out - sorry, I'm not a programmer:

In the options field there's an english-german language mixup - see the screenshot:
http://www.schultz-hamburg.de/joomla-newsflash.png
Doesn't the module find the appropriate value?
I couldn't find the file where the values are translated and assigned to the corresponding keys.

I only found the german translation file de-DE_mod_articles-news_ini where the following lines are missing:
MOD_ARTICLES_NEWS_FIELD_IMAGES_ARTICLE_DESC="... german translation ..." MOD_ARTICLES_NEWS_FIELD_IMAGES_ARTICLE_LABEL="... german translation ..."
This is not a big issue, it's only the translation of the label and the description - so I tried and added it on my site. Of course, it didn't help with the bug.

Thanks for your patience!

@ghost
Copy link

ghost commented May 13, 2019

@ute-sch please ask on Forum

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants