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.0] Replace com_content 'Image Float' with 'Image Class" #17402

Closed
wants to merge 7 commits into from

Conversation

ciar4n
Copy link
Contributor

@ciar4n ciar4n commented Aug 3, 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).

@ghost
Copy link

ghost commented Aug 4, 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.

@brianteeman
Copy link
Contributor

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.

@ciar4n
Copy link
Contributor Author

ciar4n commented Aug 4, 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).

@brianteeman
Copy link
Contributor

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.

@ciar4n
Copy link
Contributor Author

ciar4n commented Aug 4, 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.

@mbabker
Copy link
Contributor

mbabker commented Aug 4, 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).

@ciar4n
Copy link
Contributor Author

ciar4n commented Aug 4, 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

@dgrammatiko
Copy link
Contributor

dgrammatiko commented Aug 4, 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

@mbabker
Copy link
Contributor

mbabker commented Aug 4, 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.

@dgrammatiko
Copy link
Contributor

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...

@brianteeman
Copy link
Contributor

@ciar4n that should work

@ciar4n
Copy link
Contributor Author

ciar4n commented Aug 5, 2017

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

@brianteeman
Copy link
Contributor

Thanks. I cannot test ignore today but will do ASAP

@ghost
Copy link

ghost commented Aug 5, 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.

@ciar4n
Copy link
Contributor Author

ciar4n commented Aug 5, 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.

@ghost
Copy link

ghost commented Aug 5, 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.

@ReLater
Copy link
Contributor

ReLater commented Aug 5, 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.

@ciar4n
Copy link
Contributor Author

ciar4n commented Aug 6, 2017

@ReLater Fixed

@N6REJ
Copy link
Contributor

N6REJ commented Aug 10, 2017

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

@rdeutz
Copy link
Contributor

rdeutz commented Aug 18, 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

@ghost ghost added the J4 Issue label Apr 5, 2019
@ghost ghost removed the J4 Issue label Apr 13, 2019
@sanderpotjer
Copy link
Member

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

@nidhi1709
Copy link

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

@N6REJ
Copy link
Contributor

N6REJ commented Oct 25, 2019

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

@ciar4n
Copy link
Contributor Author

ciar4n commented Oct 25, 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.

@ciar4n
Copy link
Contributor Author

ciar4n commented Oct 9, 2020

Reopened here.. #31017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants