? Success

User tests: Successful: Unsuccessful:

avatar brianteeman
brianteeman
15 Oct 2016

I am not sure why but we have a layout for intro_image but not for full_image. This pr addresses that.

Testing Instructions

Create an article with an intro and a full image (from the images and links tab) and make sure that article can be accessed from the front end from a single article, featured articles, category list and category blog view
Check that the images display as expected
Apply the PR and recheck - there should be no visible change at all

This is fully B/C as the code itself has not changed its just been moved and if you have an override in your template for com_content then the override will still work

avatar brianteeman brianteeman - open - 15 Oct 2016
avatar brianteeman brianteeman - change - 15 Oct 2016
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 15 Oct 2016
Labels Added: ? ?
avatar joomla-cms-bot joomla-cms-bot - change - 15 Oct 2016
Category Front End Components Layout
avatar brianteeman brianteeman - change - 15 Oct 2016
Labels Removed: ?
avatar sovainfo
sovainfo - comment - 15 Oct 2016

Documentation:
This PR creates the layout joomla/content/full_image.php and uses it in components/com_article/views/tmp/default.php.
Overrides should be re-evaluated and this change applied or possibly partly moved to an override of the layout.

EDIT bad english effecting => affecting in the next line!
The release notes should be adjusted and report this PR as one that is affecting overrides.

Note that the last line needs a linefeed.

avatar brianteeman
brianteeman - comment - 15 Oct 2016

This does not effect overrides at all

avatar sovainfo
sovainfo - comment - 15 Oct 2016

As soon as a PR touches something that can be overridden it has an effect on overrides.
The administrator needs to re-evaluate the override because the source he based the override on changed! That is also called having an effect!
In this case my advice would be to accept this change and apply it to your override. When your own override is concerning the full_image, suggest to override the layout instead of the view layout.

Indeed, this project ignored the effects on overrides, which is why I just documented it!

avatar mbabker
mbabker - comment - 15 Oct 2016

It only has an effect on overrides by causing an administrator to re-evaluate their override needs. To say it any other way would be misinformation. Changing the core layout as this PR does does not affect existing layout overrides in any way (speaking from a resulting output standpoint, not a management standpoint as you're arguing).

avatar sovainfo
sovainfo - comment - 15 Oct 2016

Isn't that what documentation is all about? Informing people what the effect is for their site?

avatar mbabker
mbabker - comment - 15 Oct 2016

In this case though we're getting really nit-picky over things. It doesn't affect existing overrides in any way so for the vast majority of users it honestly doesn't matter if there's a big orange warning banner or if it's not documented at all because they just expect things to still work. For the few who really are paying close attention to their overrides to keep them in sync with core, odds are they're probably already doing proper diffs of the packages or watching GitHub and the added documentation really isn't going to make a difference.

In general there should probably be a page on each release's FAQ with a list of modified core layouts so folks know what's changed. But as I've been saying lately, Joomla's terrible with documenting anything outside pull requests.

avatar sovainfo
sovainfo - comment - 15 Oct 2016

Never realized contributing missing documentation was not appreciated here.
Just because Joomla project is bad at documention means we shouldn't improve it?
Can't believe the opposition, I'll withdraw again!

avatar brianteeman
brianteeman - comment - 15 Oct 2016

There is zero effect on any existing override

avatar mbabker
mbabker - comment - 15 Oct 2016

Not trying to oppose you by any means, but your first couple of comments read to me as a native English (US flavor so some don't nitpick over that too ?) speaker as though you're trying to suggest that this PR changes someone's overrides, which it doesn't. It does change whether the site needs the overrides (if someone has adjusted the full image section of the layout only then they can override the new JLayout file and remove the component layout override), and as I suggested that type of change should be included in the FAQ. But as I also implied, I've got a better chance of winning the lotto and being struck by lightning in the same day than the documentation workflows improving.

avatar brianteeman
brianteeman - comment - 15 Oct 2016

@sovainfo you could have written millions of docs by now. Suggestions are
great but if you dont pull up your sleeve and do something then thats all
they will ever be

On 15 October 2016 at 23:04, Michael Babker notifications@github.com
wrote:

Not trying to oppose you by any means, but your first couple of comments
read to me as a native English (US flavor so some don't nitpick over that
too ?) speaker as though you're trying to suggest that this PR changes
someone's overrides, which it doesn't. It does change whether the site
needs the overrides (if someone has adjusted the full image section of the
layout only then they can override the new JLayout file and remove the
component layout override), and as I suggested that type of change should
be included in the FAQ. But as I also implied, I've got a better chance of
winning the lotto and being struck by lightning in the same day than the
documentation workflows improving.


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#12421 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABPH8agnn01Kap8FP6kRfXehYAHY69Wzks5q0U3VgaJpZM4KXsBx
.

Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
https://brian.teeman.net/ http://brian.teeman.net/

avatar Hackwar
Hackwar - comment - 16 Oct 2016

Do we REALLY need a layout file for basically an img-tag? I strongly suggest reducing the number of layouts and not add more for trivial output.

avatar brianteeman
brianteeman - comment - 16 Oct 2016

That's because you don't understand how to use layouts or the benefits they
bring. I use layouts all the time and they are incredibly beneficial when
producing custom designs. This one is in some ways for consistency as we
have one for the intro image.

avatar Hackwar
Hackwar - comment - 16 Oct 2016

I understand the usage of layouts very well, thank you. But I don't think it is sane to have micro-layouts for everything. We already have quite a lot of unnecessary layouts and layouts in my opinion should have a certain amount of complexity to warant creating a standardised output for this. Two tags and 5 lines of code are not complex in my book. However, adding a layout means that for at least the 3.x branch we are stuck with that layout. We can not (really) change it nor can we remove it. At the same time, layouts do have downsides, like a loss of performance, additional files for us to maintain, an increased inode count and in general yet another level of indirection. Considering all of that, I don't think adding this layout is a good idea.

avatar brianteeman
brianteeman - comment - 16 Oct 2016

Well thats your opinion - I obviously disagree.

the think that you are completely forgetting (or maybe you dont know) is
that while the current layout might look fairly simple it doesnt have to be
and it certainly isnt by the time I have finished overriding it.

In addition because it is a layout I can do the customisation in one place
and then reuse it elsewhere. this makes life much easier and faster for the
designer and is very DRY which as a developer you should be appreciating.

On 16 October 2016 at 17:13, Hannes Papenberg notifications@github.com
wrote:

I understand the usage of layouts very well, thank you. But I don't think
it is sane to have micro-layouts for everything. We already have quite a
lot of unnecessary layouts and layouts in my opinion should have a certain
amount of complexity to warant creating a standardised output for this. Two
tags and 5 lines of code are not complex in my book. However, adding a
layout means that for at least the 3.x branch we are stuck with that
layout. We can not (really) change it nor can we remove it. At the same
time, layouts do have downsides, like a loss of performance, additional
files for us to maintain, an increased inode count and in general yet
another level of indirection. Considering all of that, I don't think adding
this layout is a good idea.


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#12421 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABPH8dJVQVCKJXYllPL-Nv5cfD--bQXqks5q0k1AgaJpZM4KXsBx
.

Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
https://brian.teeman.net/ http://brian.teeman.net/

avatar Hackwar
Hackwar - comment - 16 Oct 2016

I understand you, although you did not convince me. This is one of the moments where I'm happy that I'm not in a position to having to decide this. ? Looking forward to a decision by the maintainers.

avatar brianteeman
brianteeman - comment - 16 Oct 2016

/me Looking in the mirror

avatar brianteeman
brianteeman - comment - 16 Oct 2016

If you had sensible objections it would help your cause. Saying one extra
inside is a problem is just a joke

avatar Hackwar
Hackwar - comment - 16 Oct 2016

In the end, we have to start somewhere.

avatar brianteeman
brianteeman - comment - 16 Oct 2016

Let me give you a super simple real world example from today.
I customised the layout if the intro image and I want to reuse that in a
single article view.
Without this pr i have to create an override of the article view. Locate
the spaghetti code for the full image. Delete that and insert the one line
for the intro image layout.
With this pr i create an override of the article view and change five
characters (full->intro) and job is done.
At the end of the day I submitted a simple pr that works and makes users
life easier. It has zero backward compatibility issue and doesn't require
hours/days of testing and fixing and then refactoring.

(I was not paid to create this pr. I could have kept it for myself but I
believe in contributing freely so that others can benefit)

avatar brianteeman brianteeman - change - 29 Oct 2016
Labels Added: ?
Removed: ?
avatar brianteeman brianteeman - change - 4 Nov 2016
Easy No Yes
avatar brianteeman brianteeman - edited - 4 Nov 2016
avatar coolcat-creations
coolcat-creations - comment - 6 Dec 2016

tested successfully - can´t submit it at the top because of error


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

avatar jeckodevelopment jeckodevelopment - alter_testresult - 6 Dec 2016 - coolcat-creations: Tested successfully
avatar jeckodevelopment
jeckodevelopment - comment - 6 Dec 2016

@coolcat-creations saved your test

avatar allrude
allrude - comment - 6 Dec 2016

I agree with Brain that that this is a useful addition, in fact i'm using a Jlayout for fulltext-images for more then a year now, never came to mind sharing it, so kudos Brain.

I can understand Hannes his point that we shouldn't have microlayouts for every single option,
but for the items/options in the images and links tab, this for sure is a welcome addition,
we can reuse the view not only in Article and Blog layouts of com_content, but also in content modules, making as Brian stated the code reusable, maintainable and DRY.

so +1 from me for this PR

avatar alikon
alikon - comment - 6 Dec 2016

I have tested this item successfully on 44096e2


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

avatar alikon alikon - test_item - 6 Dec 2016 - Tested successfully
avatar jeckodevelopment jeckodevelopment - change - 6 Dec 2016
Status Pending Ready to Commit
avatar jeckodevelopment
jeckodevelopment - comment - 6 Dec 2016

RTC


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

avatar jeckodevelopment jeckodevelopment - change - 6 Dec 2016
Milestone Added:
avatar rdeutz rdeutz - change - 6 Dec 2016
Status Ready to Commit Fixed in Code Base
Closed_Date 0000-00-00 00:00:00 2016-12-06 21:54:33
Closed_By rdeutz
avatar rdeutz rdeutz - close - 6 Dec 2016
avatar rdeutz rdeutz - merge - 6 Dec 2016
avatar rdeutz rdeutz - reference | 20d5ddd - 6 Dec 16
avatar rdeutz rdeutz - merge - 6 Dec 2016
avatar rdeutz rdeutz - close - 6 Dec 2016
avatar joomla-cms-bot joomla-cms-bot - change - 6 Dec 2016
Category Front End Components Layout Front End com_content Layout Components
avatar brianteeman
brianteeman - comment - 6 Dec 2016

Thanks

avatar brianteeman brianteeman - head_ref_deleted - 6 Dec 2016

Add a Comment

Login with GitHub to post a comment