? ? ? Failure

User tests: Successful: Unsuccessful:

avatar ciar4n
ciar4n
3 Aug 2017

Pull Request for Issue #17399 .

Summary of Changes

Replaces the 'Image Float ' field in com_contents with an 'Image Class' field (For reasons why see the original issue.. #17399)

Testing Instructions

Navigate to the Images and Links tab in article edit. Set an image and add a utility class to the Image Class field (eg. float-right). Check frontend and ensure class has been applied to the image (eg. image floated to right / inspect element).

avatar joomla-cms-bot joomla-cms-bot - change - 3 Aug 2017
Category Administration com_content Language & Strings Layout
avatar ciar4n ciar4n - open - 3 Aug 2017
avatar ciar4n ciar4n - change - 3 Aug 2017
Status New Pending
avatar franz-wohlkoenig franz-wohlkoenig - test_item - 4 Aug 2017 - Tested successfully
avatar franz-wohlkoenig
franz-wohlkoenig - comment - 4 Aug 2017

I have tested this item successfully on 2be532b


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

avatar brianteeman
brianteeman - comment - 4 Aug 2017

The problem I see with this change is that

  1. it means that we will end up with unused params in the database on upgraded web site with no way to remove them without editing the database directly

  2. This is changing users data

  3. This will require a site owner to review all their existing content and to add in the class manually. On my blog for example I would have to do this for thousands of articles

  4. This will require every template to be updated

Just because we can break backwards compatibility does not me we should do it without carefully considering the consequences.

avatar ciar4n
ciar4n - comment - 4 Aug 2017

This is largely true for any change to the options. How has this been handled in the past?

I made my reasons as best I could in the original issue (#17399).

avatar brianteeman
brianteeman - comment - 4 Aug 2017

Off the top of my head I can't think of an example of where an option has been removed that effects user data like this. Probably because it hasn't been done for the reasons I outlined above.

avatar ciar4n
ciar4n - comment - 4 Aug 2017

This option as it is, should never have been added in the first place. Tying an option in core directly to any CSS property is wrong and will always cause a b/c break at some point. Changing this to Image Class means we never have to worry about a b/c break in instances like this again. It creates a layer that can be changed as CSS changes.

avatar mbabker
mbabker - comment - 4 Aug 2017

Fact is it's there now and it's our responsibility now to deal with how to go forward with it. Unfortunately we are approaching uncharted territory since we historically have avoided changes that impact user data and this change would need a migration tool to "fix" things the right way (granted it could be done with a single PHP file with a couple of prompts to convert a float to a CSS class, but it still couldn't be automated).

avatar ciar4n
ciar4n - comment - 4 Aug 2017

The only solution I can suggest within my capabilities is to use the old param names (float_intro & float_fulltext). Value saved for each of these is left and right so I can just define these as classes in the CSS to float left and right respectively...

.left.item-image {
    float: left;
}
.right.item-image {
    float: right;
}

Not the 'cleanest' solution but will resolve all issues mentioned by @brianteeman

avatar dgrammatiko
dgrammatiko - comment - 4 Aug 2017

@mbabker I think we have to figure (no pun indented) out something for images one way or another: #16948

So I guess what @ciar4n is suggesting here, should be part of the bigger, known problem

avatar mbabker
mbabker - comment - 4 Aug 2017

Well aware, I will say though that solution will probably have to be a standalone tool because we should not try to blindly migrate user data unless we have 100% certainty our upgrade process can handle it without screwing existing user sites up.

And even then, I'd still do it standalone for the simple fact if we're talking about a site with a large quantity of content, we're going to be much more prone to running into resource issues if we're doing that change in the middle of the regular update routine.

avatar dgrammatiko
dgrammatiko - comment - 4 Aug 2017

I know that both you and George are aware of the problem, all I wanted to point out is that if we are going to fix the captions legacy somehow then we should consider this proposal as well. Looking ahead this could be very beneficial for all devs...

avatar brianteeman
brianteeman - comment - 4 Aug 2017

@ciar4n that should work

avatar joomla-cms-bot joomla-cms-bot - change - 5 Aug 2017
Category Administration com_content Language & Strings Layout Administration com_content Language & Strings Templates (admin) SQL Installation Postgresql Layout Front End Templates (site) Unit Tests
avatar ciar4n ciar4n - change - 5 Aug 2017
Labels Added: ? ?
avatar ciar4n
ciar4n - comment - 5 Aug 2017

Amended the PR as I suggested here #17402 (comment)

avatar brianteeman
brianteeman - comment - 5 Aug 2017

Thanks. I cannot test ignore today but will do ASAP

avatar franz-wohlkoenig franz-wohlkoenig - test_item - 5 Aug 2017 - Tested successfully
avatar franz-wohlkoenig
franz-wohlkoenig - comment - 5 Aug 2017

I have tested this item successfully on 0856d23

  • Article, Full Image using "Use global (left)": after PR Image stay left even "Image Class" is empty
  • apply "float-right" in "Image Class" Article shows in Frontend Image right aligned.

This didn't work on Intro-Image as it was in Joomla 3 too ("Use global (*)" didn't change alignment) which is not Scope of this PR.


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

avatar ciar4n
ciar4n - comment - 5 Aug 2017

@franz-wohlkoenig Thank you for the test

Article, Full Image using "Use global (left)": after PR Image stay left even "Image Class" is empty

Check your global config as this is set to 'left' by default with the current sample data.

avatar franz-wohlkoenig
franz-wohlkoenig - comment - 5 Aug 2017

@ciar4n i meant that this behaviour is fine as discussed above – so updated Homepages don't have to be controlled if Articles-Images are right aligned.

avatar ciar4n ciar4n - change - 5 Aug 2017
Labels Added: ?
avatar ReLater
ReLater - comment - 5 Aug 2017

When I enter "right" for the full image I get a class "right item-image" via layouts/joomla/content/full_image.php.

When I enter "right" for the intro image I get a class "pull-right item-image" via layouts/joomla/content/intro_image.php.

avatar ciar4n
ciar4n - comment - 6 Aug 2017

@ReLater Fixed

avatar N6REJ
N6REJ - comment - 10 Aug 2017

following and will test/feedback tomorrow or late tonight since its class night.

avatar rdeutz
rdeutz - comment - 18 Aug 2018

for the test can you make a PR against this Repo https://github.com/joomla/test-integration files are in the 4.0-dev branch /stubs/database and remove the files from your PR here, thanks

avatar franz-wohlkoenig franz-wohlkoenig - change - 18 Aug 2018
Status Pending Information Required
avatar franz-wohlkoenig franz-wohlkoenig - change - 11 Apr 2019
Category Administration com_content Language & Strings Layout Templates (admin) SQL Installation Postgresql Front End Templates (site) Unit Tests Administration com_content Front End Installation Layout Postgresql SQL Templates (admin) Templates (site) Unit Tests
avatar sanderpotjer
sanderpotjer - comment - 5 May 2019

@ciar4n can you update this PR against the latest 4.0-dev branch to solve the merge conflicts? Thanks in advance!

avatar nidhi1709
nidhi1709 - comment - 19 Oct 2019

Error while applying patch..Wasn't able to test the issue

avatar N6REJ
N6REJ - comment - 25 Oct 2019

whats the matter with using the default bootstrap "pull-right/pull-left? which if I recall doesn't do much in grid

avatar ciar4n
ciar4n - comment - 25 Oct 2019

In general, since we now have flex, floating elements should be avoided considering the negative effect it can have surrounding elements.

Probably easier creating a new pr than bring this one up to date so gonna close.

avatar ciar4n ciar4n - change - 25 Oct 2019
Status Information Required Closed
Closed_Date 0000-00-00 00:00:00 2019-10-25 08:48:11
Closed_By ciar4n
avatar ciar4n ciar4n - close - 25 Oct 2019
avatar joomla-cms-bot joomla-cms-bot - change - 25 Oct 2019
Category Administration com_content Layout Templates (admin) SQL Installation Postgresql Front End Templates (site) Unit Tests Administration com_content Language & Strings Templates (admin) SQL Installation Postgresql Layout Front End Templates (site) Unit Tests
avatar ciar4n
ciar4n - comment - 9 Oct 2020

Reopened here.. #31017

Add a Comment

Login with GitHub to post a comment