User tests: Successful: Unsuccessful:
Labels |
Labels |
Added:
?
|
Well it depends. But the same clearfix
we have in the featured view ;)
Good point
I'll leave that to frontend experts (which I'm not).
The problem is your content not joomla. The same thing could happen for any article not just leading
@brianteeman but we have this clearfix
historically. See this 62d222b - Seth removed clear div, but forgot to add that clearfix
class for a blog view, but did it for a featured
view.
So anyone have more opinions here? :) For me it is good to merge :p
@nternetinspired Seth, since it was originally your change. Was there a reason you left out this div?
IMO, the clearfix class shouldn't be in the featured view item, it was a mistake on my part. I think it should be removed.
I agree that this is a content issue. If you float an element in your content it should behave like that. If you are going to add floats inside your content and you then want them cleared, you should be clearing them in your content too I think.
Agree with @b2z. This PR should be applied.
Indeed @nternetinspired made a mistake, his pr introduced a bc issue and should not have been merged. It did not deliver on its promiss to only remove unneeded empty divs. Also it introduced inconsistancy.
Even when it is decided that it is a content issue, changes in functionality should not be introduced like this. Can't believe people still suggesting changes that require work on existing sites. Bad enough when that happens when fixing bugs.
I disagree. We could clearfix everything to prevent content issues breaking layouts, but that would be incorrect, it doesn't rectify the root of such issues.
Moreover, it's further unneeded complication of the markup. Ideally clearfix should never appear in the markup at all, there is to much potential for unintentionally breaking of intended layout patterns.
There is simply no reason to use <p class="pull-left within the content, that is what's causing the issue. In an element was unclosed in the content, e.g: <h2Header oops<h2 that would also affect the following content in an undesired way. It's no different to what is happening in this example.
Because it breaks BC such a change would have to wait for J4. Hopefully it will be done properly then, and not disguised/hidden/inconsistently.
Currently there are 76 occurences of clearfix in j3.2.3, of which 55 are the empty divs that were removed.
Hope the impact is limited, so that change is not added to the list of bloopers.
Yeap. It will be a B/C break, agree.
I'd say let's put that clearfix
in so it's at least consistently wrong.
And then get rid of them in a future release everywhere where it's not really needed.
I disagree. Clearfixing elements just for the sake of it is both unnecessary and undesirable, adding bloat when we should be looking to remove it. In this case it's ridiculous.
Would you clearfix the module wrapper, just in case someone improperly uses a float inside a module? Or a contact info field? What about a menu item?? All of these things, and more, can be broken by someone adding floats improperly. That isn't sufficient justification to clearfix every element we can find.
This PR also overlooks the fact that the observed behaviour is exactly what is supposed to happen. That is how floats work, that is exactly what the float property should be doing. Clearfixing is a hack, useful in some circumstances, but a hack nevertheless.
Clearfixing is a hack, useful in some circumstances, but a hack nevertheless.
I agree fully.
But the thing is that currently, it is inconsistent between the featured layout (where it's present) and the blog layout (where it's not present). It's basically two layouts which do the exact same, and they should have the same classes. Otherwise it makes no sense.
Agreed 100%. I also think the best way forward is to remove that inconsistency, but I'd prefer to do it by removing the clearfix class on line 32 of the featured view.
Of course, the clearfix on line 30 would need to remain for the time because removing that one would cause a logical BC break, I think that many templates rely on it for article layout purposes.
And the same argument applies to the other clearfix as b2z made clear.
So? What will be the final decision here? :)
Labels |
Removed:
?
|
Labels |
Status | New | ⇒ | Pending |
Category | ⇒ | Template |
Let's say we remove the clearfix on the featured view as well. What is the final fix for getting the header to move left?
What is the final fix for getting the header to move left?
Don't float the preceding div.
@b2z I meant how to come to a solution for this and get it tested and merged. The answer by @nternetinspired is Greek to me but perhaps it is enough for you to continue on this PR?
@nternetinspired can you please elaborate? There are no floating divs in the layout.
The original issue reported was a BC break by @nternetinspired removing obsolete html code. Turns out it wasn't obsolete and needed a clearfix tag (as provided in Featured blog, but left out for Category blog). Instead of admitting his mistake, he now claims doing the right thing in Featured blog was his mistake!
With that he admits he tried to deceive intentionally by claiming to remove redundant code, but actually wants to change it. Obviously, that change is not desired.
Consider it the responsibility of the Category/Featured blog to provide the expected output. Initially that was done using clearfix. Removing that makes the site look like having layout issues. That means site owners need to change all content because Joomla needed to break their site AGAIN!
See no need to continue that bad reputation!
With that he admits he tried to deceive intentionally by claiming to remove redundant code, but actually wants to change it. Obviously, that change is not desired.
I want to see bad, unnecessary and ill-considered code removed, yes. This is how improvements are made.
I've have never tried to deceive anyone here. Your accusation that I have intentionally tried to deceive is both untrue and unwarranted, as well as quite unbelievable. Let's stick to discussions about code and commits, rather than make personal attacks, shall we?
Please lets try not to make personal comments and comment ONLY on the code
On 15 May 2015 at 11:08, Seth Warburton notifications@github.com wrote:
With that he admits he tried to deceive intentionally by claiming to
remove redundant code, but actually wants to change it. Obviously, that
change is not desired.I want to see bad, unnecessary and ill-considered code removed, yes. This
is how improvements are made.I've have never tried to deceive anyone here. Your accusation that I have
intentionally tried to deceive is both untrue and unwarranted, as well as
quite unbelievable. Let's stick to discussions about code and commits,
rather than make personal attacks, shall we?—
Reply to this email directly or view it on GitHub
#3240 (comment).
Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
http://brian.teeman.net/
Just stating facts anybody can verify. As explained, the initial PR claimed to remove redundant code only. It turns out it is not redundant, applying the PR changed behaviour. Removing the so called redundant code had an undesired effect, which unfortunately wasn't detected. Can only hope it wouldn't have been committed if the inconsistency and changed behaviour was detected before. Assuming it would be corrected without changing its behaviour.
With the easy fix as proposed by @b2z there is also no need to revert that PR. Just appy the tag where it is needed.
Obviously, the original author considered it the responsibility of the blog to make sure articles are presented as desired and contents on articles don't mess up the blog layout.
Feel free to disagree on that, but don't change the rules of the game while playing!
Consider it deceiving claiming to remove redundant code while intending to change behaviour. The behaviour shouldn't change.
So lets leave this clearfix Just synced the branch.
Try to reproduce it, but I don't see the Problem.
(The Link is broken incidentally...) Sorry...
Unfortunatelly I couldn't reproduce the issue. Given link is broken
@hitchblade @Teck95 what link do you mean, http://joomlacode.org? There is nothing there. The issue created here is the same as there. But basically nothing to test here ;) Can we just merge it?
@b2z Not sure what link they mean either but they said that the issue cannot be reproduced.
Category | ⇒ | Templates (site) |
Thank you for your contribution but it has been decided that this is not something that will be included in the core of Joomla! at the moment.
Status | Pending | ⇒ | Closed |
Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2016-05-07 17:04:56 |
Closed_By | ⇒ | Kubik-Rubik |
Who are you talking to @Kubik-Rubik? There are several contributors here…
Also, maybe good if you elaborate on the decision. Bug fixes will not be included in core at the moment??
Isn't the issue here more your content than the blog layout?
You have a
<div class="pull-left">
around your short paragraph which makes the second header correctly float around it. Imho better would be to clear the floating in your content, since you introduced the floating there. Or am I wrong?