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 iframe Answers Map/List toggle button on Mobile Error #1150

Merged
merged 5 commits into from
Sep 27, 2023

Conversation

jesuyedavid
Copy link
Contributor

Iframe needs height in order for map and toggle button to be initialized and to fill js-answersMap

Iframe needs height in order for map and toggle button to be initialized and to fill js-answersMap
@coveralls
Copy link

coveralls commented Sep 25, 2023

Coverage Status

coverage: 8.737%. remained the same when pulling d47ab5b on hotfix/v1.33.1 into 0222c71 on master.

@@ -70,6 +70,8 @@ export function generateIFrame(domain, answersExperienceFrame) {
iframe.frameBorder = 0;

// For dynamic iFrame sizing
iframe.style.height = '100%';
iframe.style.minHeight = '100%';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a little uncertain about this solution. There are a lot of potential problems with setting 100% height.

Can we take a look at this with a test across a few different browsers?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should first look at upgrading iframe-resizer which is responsible for resizing the iframe to fill the view height. It's possible there is a bug there: https://www.npmjs.com/package/iframe-resizer

el.style.height = `${el.scrollHeight}px`
}
}
//NOTE: set min-height for js-answersMap instead to prevent keyboard from shifting the page
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we don't need this code we can delete it instead of commenting it out

cea2aj
cea2aj previously approved these changes Sep 27, 2023
Copy link
Member

@cea2aj cea2aj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! We can ignore the percy and browserstack errors since we don't use those services anymore

@jesuyedavid jesuyedavid merged commit 6a472ed into master Sep 27, 2023
17 of 28 checks passed
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

4 participants