User tests: Successful: Unsuccessful:
I am not sure why but we have a layout for intro_image but not for full_image. This pr addresses that.
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
Status | New | ⇒ | Pending |
Labels |
Added:
?
?
|
Category | ⇒ | Front End Components Layout |
Labels |
Removed:
?
|
This does not effect overrides at all
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!
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).
Isn't that what documentation is all about? Informing people what the effect is for their site?
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.
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!
There is zero effect on any existing override
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
@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/
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.
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.
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.
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/
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.
/me Looking in the mirror
If you had sensible objections it would help your cause. Saying one extra
inside is a problem is just a joke
In the end, we have to start somewhere.
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)
Labels |
Added:
?
Removed: ? |
Easy | No | ⇒ | Yes |
tested successfully - can´t submit it at the top because of error
@coolcat-creations saved your test
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
I have tested this item
Status | Pending | ⇒ | Ready to Commit |
RTC
Milestone |
Added: |
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 |
Category | Front End Components Layout | ⇒ | Front End com_content Layout Components |
Thanks
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.