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

fix embed overflow beyond iframe #87

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

Muhammad-Owais-Warsi
Copy link

What should this PR do?

Fixed some styling in order to prevent embed overflow beyond iframe.

What issue does this relate to?

#60

What are the acceptance criteria?

@erhilse
Copy link
Contributor

erhilse commented Oct 25, 2024

👋 @Muhammad-Owais-Warsi while this fixes the initial issue it doesn't really solve the problem. Can you review the expected behavior again and if none of those options work can you provide a record of testing then I can accept this PR.

@Muhammad-Owais-Warsi
Copy link
Author

👋 @Muhammad-Owais-Warsi while this fixes the initial issue it doesn't really solve the problem. Can you review the expected behavior again and if none of those options work can you provide a record of testing then I can accept this PR.

Hey @erhilse , the changes I made, give me the following result

Screenshot from 2024-10-25 22-58-36
Screenshot from 2024-10-25 23-01-31

Let me know if I misunderstood something.

@erhilse
Copy link
Contributor

erhilse commented Oct 25, 2024

@Muhammad-Owais-Warsi the issue mentions this:

Expected behavior

Embed is sized correctly for the content within the iframe, ideally by talking to the iframe to get the height of the content within (see if OpenCollective has a documented way to do this using messaging, or potentially open an issue/PR in an appropriate repository on their side to implement such a thing).

Ideally we get the height of the iframe dynamically instead of hardcoding a value that could potentially change.

@Muhammad-Owais-Warsi
Copy link
Author

@Muhammad-Owais-Warsi the issue mentions this:

Expected behavior

Embed is sized correctly for the content within the iframe, ideally by talking to the iframe to get the height of the content within (see if OpenCollective has a documented way to do this using messaging, or potentially open an issue/PR in an appropriate repository on their side to implement such a thing).

Ideally we get the height of the iframe dynamically instead of hardcoding a value that could potentially change.

Oh! Thanks @erhilse for clearing the doubt. I''ll look, on how to fetch the content height and change the iframe height dynamically.

@Muhammad-Owais-Warsi
Copy link
Author

Muhammad-Owais-Warsi commented Oct 26, 2024

Hey @erhilse,
I checked the Open Collective documentation, and I don’t think they provide a height parameter to dynamically set the iframe's height. I just want to confirm if it’s okay to allow the content inside the iframe to be scrollable and not chnaging the height param.

@erhilse
Copy link
Contributor

erhilse commented Oct 29, 2024

@Muhammad-Owais-Warsi - We are trying to avoid the scrolling and cutting off content. You could open a PR on OpenCollectives who are also participating in Hacktoberfest to implement the feature needed.

@Muhammad-Owais-Warsi
Copy link
Author

@Muhammad-Owais-Warsi - We are trying to avoid the scrolling and cutting off content. You could open a PR on OpenCollectives who are also participating in Hacktoberfest to implement the feature needed.

Thanks @erhilse , will lookout to their repo.

@Muhammad-Owais-Warsi
Copy link
Author

Hey @erhilse, the Open Collective updated and provide dimensions of the iframe along with the other params. They didn't deployed it yet. Once deployed I'll make the suitable changes. You can checkout the changes made by Open Collective here.

@Muhammad-Owais-Warsi
Copy link
Author

It's live and tested it, but I think the issue persits.

@erhilse
Copy link
Contributor

erhilse commented Jan 2, 2025

@Muhammad-Owais-Warsi - it doesn't look like you've added in any code to support the new feature. You'll need to set up an event listener like they have in their PR to get the value once the event has fired.

@Muhammad-Owais-Warsi
Copy link
Author

@Muhammad-Owais-Warsi - it doesn't look like you've added in any code to support the new feature. You'll need to set up an event listener like they have in their PR to get the value once the event has fired.

Actually I didn't updated the PR yet. I'll update and will let you know. Thanks.

@Muhammad-Owais-Warsi
Copy link
Author

Hey @erhilse, I've made the changes, please do review and let me know the changes if needed.

@erhilse
Copy link
Contributor

erhilse commented Jan 15, 2025

Hey @Muhammad-Owais-Warsi - Sorry I haven't gotten back to you. I was reviewing your PR and came across another issue and wanted to discuss with the team. The iframe now loads at the correct size but when you are interacting with it and go to the next screen it resizes which is good but if you go back a step it miscalculates the step. Another thing I came across was there is no event to track when the faq open/close so if you open one it gets cut off on the iframe. That being said I think we are going to move forward with the approach you have but just ask that on the iframe you remove the scrolling="no". This will at least allow the iframe to be a good size when its loaded and allow for scrolling if the content is too large to fit in that area.

@Muhammad-Owais-Warsi
Copy link
Author

Hey @erhilse, thanks for reviewing the PR and pointing out the issues. Just to clarify, I only need to remove scrolling="no" , right?

@erhilse
Copy link
Contributor

erhilse commented Jan 16, 2025

Yeah, that should allow us to scroll if the content is larger than the iframe

@Muhammad-Owais-Warsi
Copy link
Author

I have updated the code, please review and let me know the issues if any. Thanks!

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.

2 participants