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

Resize to the correct height of the iframe #11554

Merged
merged 2 commits into from
Nov 15, 2016

Conversation

laoneo
Copy link
Member

@laoneo laoneo commented Aug 11, 2016

Summary of Changes

Auto height is broken for the wrapper menu item.

Testing Instructions

  • Create a wrapper menu item with a local link like demo/1-article.
  • In the advanced options of the wrapper menu item set Auto Height to yes.
  • Open the wrapper menu item on the front.

Expected result

The iframe is resized that not scrollbars are visible.

Actual result

The iframe doesn't get resized.

@brianteeman
Copy link
Contributor

This needs to be tested fully on all browsers, especially IE, as the iframe stuff works differently across the browsers

@laoneo
Copy link
Member Author

laoneo commented Aug 11, 2016

I'v developed it with Chrome on Ubuntu Linux.

@brianteeman
Copy link
Contributor

Hence the need for extensive testing - this is based on experience ;)

On 11 August 2016 at 12:32, Allon Moritz notifications@github.com wrote:

I'v developed it with Chrome on Ubuntu Linux.


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

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

@C-Lodder
Copy link
Member

Not working on IE10 or below

@C-Lodder
Copy link
Member

Update the code to this:

function iFrameHeight()
{
    var height = 0;
    var iframe = document.getElementById('blockrandom');
    var doc    = 'contentDocument' in iframe ? iframe.contentDocument : iframe.contentWindow.document;

    if (!document.all)
    {
        height = doc.body.scrollHeight;
        iframe.style.height = parseInt(height) + 60 + 'px';
    }
    else if (document.all)
    {
        height = doc.body.scrollHeight;
        document.all.blockrandom.style.height = parseInt(height) + 20 + 'px';
    }
}

Works on all browsers. There is a slight scroll on IE8 still, so whether or not you want to extend the above code to fix full or leave it is up to you

@laoneo
Copy link
Member Author

laoneo commented Aug 11, 2016

Updated the code as suggested by @C-Lodder. Thanks for that!

@C-Lodder
Copy link
Member

I have tested this item ✅ successfully on 4c213a3


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

@jeckodevelopment
Copy link
Member

jeckodevelopment commented Aug 11, 2016

Please test on different browsers:

  • Microsoft Edge on Windows 10
  • IE 11 on Windows
  • Mozilla Firefox (latest) on Windows
  • Mozilla Firefox (latest) on MacOS
  • Mozilla Firefox (latest) on GNU/Linux
  • Google Chrome on Windows
  • Google Chrome on MacOS
  • Opera (latest) on Windows
  • Chromium on Linux
  • Safari on Windows
  • Safari on MacOS
  • any other welcome

@C-Lodder
Copy link
Member

  • IE8 - nearly successful (not 100% height but nearly)
  • IE9 - successful
  • IE10 - successful
  • IE11 - successful
  • Edge : Win 10 - successful
  • Firefox : Windows - successful
  • Chrome : Windows - successful
  • Opera : Windows - successful
  • Yandex : Windows - successful

I don't have a Mac or Linux but all browsers (apart from slight issue on IE8) work perfectly fine.

Please note in future we must test on Yandex as Joomla states they support it.

@jeckodevelopment
Copy link
Member

AFAIK Yandex Browser is based on Chromium, so compatibility should be mostly the same.
Btw, thank you @C-Lodder for introducing me this browser :)

@brianteeman
Copy link
Contributor

brianteeman commented Aug 11, 2016 via email

@C-Lodder
Copy link
Member

@jeckodevelopment - same goes for Opera ;)
@brianteeman - Only on Mac though, not Windows

@jeckodevelopment
Copy link
Member

@brianteeman thank you!

@laoneo
Copy link
Member Author

laoneo commented Aug 12, 2016

As I'm not allowed to do testing, so just a remark. It works on Ubuntu with Firefox and Chrome. But please somebody else should confirm.

@C-Lodder
Copy link
Member

C-Lodder commented Aug 12, 2016

Macs at work so have tested on them:

  • Firefox : Mac - successful
  • Chrome : Mac - successful
  • Safari : Mac - successful

@brianteeman
Copy link
Contributor

:( unable to test with patchtester

Could not connect to GitHub: No commit found for the ref iframe-auto-height


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

@brianteeman
Copy link
Contributor

I have not tested this item.


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

@brianteeman
Copy link
Contributor

ignore that message it was a bug in patchtester 3

@brianteeman
Copy link
Contributor

brianteeman commented Nov 15, 2016

I have tested this item ✅ successfully on 4c213a3

- [ ] chrome osx

@C-Lodder
Copy link
Member

RTC?

@zero-24 zero-24 added this to the Joomla 3.7.0 milestone Nov 15, 2016
@zero-24 zero-24 added the RTC This Pull Request is Ready To Commit label Nov 15, 2016
@brianteeman
Copy link
Contributor

@C-Lodder patience ;)

@dgrammatiko
Copy link
Contributor

@laoneo since you are here can you put a deprecation notice about the blockrandom id, so in J4 we can introduce a real random id or class?

@rdeutz rdeutz merged commit e6b85d1 into joomla:staging Nov 15, 2016
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Nov 15, 2016
@laoneo
Copy link
Member Author

laoneo commented Nov 16, 2016

@dgt41 I guess the bot called rdeutz faster, the merge is done already 😢

@laoneo laoneo deleted the iframe-auto-height branch November 16, 2016 06:30
@rdeutz
Copy link
Contributor

rdeutz commented Nov 16, 2016

Thanks for calling me a bot, great motivation to spend my evening to check and merge PR

@dgrammatiko
Copy link
Contributor

@rdeutz I think Alon meant that the bot ping you, not you are a bot named rdeutz 😄

@laoneo
Copy link
Member Author

laoneo commented Nov 16, 2016

Yes I was refering the joomla bot who is adding the RTC labelt. Fixed my comment, was too early in the morning, should not try then to make jokes.

@brianteeman
Copy link
Contributor

It is not a BOT adding the label it is a human!!

There are several of us that manually review a PR before it is marked as RTC and between us we check many times a day.

Please don't scream for something to be RTC within minutes of a test. In many case we don't even get the time to get the email for a successful test before we get the shouting.

Although a PR nominally needs just two testers we do also apply some discretion on that to prevent two people in the same office or even same family from saying they have tested stuff.
Marking something as RTC is usually not just a case of flipping a switch

If it doesnt get marked RTC within 24 hours then please comment - there may be a valid reason but it may have been missed but I dont think we have missed more than 1 or 2 of the thousand plus PRs

@zero-24
Copy link
Contributor

zero-24 commented Nov 16, 2016

@dgt41 @laoneo i'm not sure what is missing now. But there should be no problem just send the missing stuff as new PR?

@laoneo
Copy link
Member Author

laoneo commented Nov 17, 2016

@brianteeman don't know what you are talking about, but I always tough that this is some hook script and not a real human
image

The whole thing got merged before I could add the code change suggested by @dgt41. I was only talking about this and not more. Don't wanted to offend anybody. Hope all clear now and we can move on.

@zero-24
Copy link
Contributor

zero-24 commented Nov 17, 2016

The bot do this on our command. Some of the people that have the permission tonset RTC does not have the permission to change labes on github. So we have implemented a bot that can do that too.

@xvinaumx
Copy link

Sorry to revive this but I feel that I should share this:

I was encountering a problem with Joomla iframe wrapper. The auto height was working in IE (Version 11.608.15063.0) but not in Chrome (Version 61.0.3163.100 - it was passing just the "60px" part). After googling a lot and finding this page, I've found the js in Joomla directory and tried to find a solution. What worked to me was changing "height = doc.body.scrollHeight" to "height = document.body.scrollHeight". Now its working in IE and Chrome.

Regards


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

@ghost
Copy link

ghost commented Sep 24, 2017

@xvinaumx can you please open a new Issue as writng on closed Issues get low Attention.

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

Successfully merging this pull request may close these issues.

None yet

9 participants