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

keep parent's domain for tab navigation on new tab in iframe experience #1067

Merged
merged 10 commits into from
May 6, 2022

Conversation

yen-tt
Copy link
Contributor

@yen-tt yen-tt commented May 4, 2022

This pr ensures tab navigation open to a new tab in an iframe experience should still be under the domain of the iFrame's parent for the new page's url.

After ANSWERS is initialized or in onReady stage, when iframeLoaded promise is resolved - indicating this is an iframe experience - a message is sent to the parent (in iframe-messaging.js) to send its url to the iframe telling it to update its tab navigation (iframe-common.js).

In the iframe's onMessage hook, setParentUrl is invoked for every navigation component on the page to properly update the tabs' link to navigate under the domain of the iframe's parent. This require changes in SDK pr. acceptance test in this pr will fail until SDK changes is merge to develop

J=SLAP-2089
TEST=manual

Tested along with the changes in theme.
Deployed two test sites in storm, with one being from the theme dev branch that points to the SDK pr dev branch, under the staging commit: dev/tab-nav-in-iframe@65f09b10. That is used in the src script of iframe_test.html of the other site under the staging commit: dev/yt-testing@517e852f. Navigate to different tabs in the same tab and opening them in new tab. See that the page is still under the domain of the iFrame's parent.

…m iframe resizer onmessage approach when answers is initialized
@yen-tt yen-tt added the WIP label May 4, 2022
@coveralls
Copy link

coveralls commented May 4, 2022

Coverage Status

Coverage decreased (-0.005%) to 8.792% when pulling 35fb0ec on dev/tab-nav-in-iframe into 31df08d on develop.

@yen-tt yen-tt removed the WIP label May 4, 2022
layouts/html.hbs Outdated
Comment on lines 162 to 166
Array.from(navItems).forEach(navItem => {
const verticalUrl = navItem.pathname.replace(/^\//, '');
const params = navItem.search ? navItem.search : '?';
navItem.href = parentUrlWithoutParams + params + '&verticalUrl=' + verticalUrl;
navItem.target = '_parent';
Copy link
Member

@cea2aj cea2aj May 4, 2022

Choose a reason for hiding this comment

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

I would expect to find the code which defines the navItem hrefs inside the navigation component. If someone is reading the navigation component code, it might be confusing to because the navigation component code doesn't include the parentURL, however in practice, it does. They probably wouldn't know to look here to find the code which updates the URLs.

Is it possible to add a function to the Navigation component which would allow you to set the parent URL? That way the Navigation component could take care of updating the URLs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wasn't sure if we want SDK changes as well. I think it's possible? I can try it out

Copy link
Contributor

Choose a reason for hiding this comment

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

We could probably add it somewhere to the addComponent call for the Navigation component (templates/common-partials/script/navigation.hbs), maybe in an onMount? I think adding new config to Navigation might also be a good choice if doing this purely in the theme gets too hacky, though.

Copy link
Contributor Author

@yen-tt yen-tt May 5, 2022

Choose a reason for hiding this comment

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

Having it in the component navigation template is tricky when it comes to making sure iframe resizer is initialized so we can get the parent url before updating the tab links. I made changes to SDK and updated this pr to invoke a function to set up the parent url!

layouts/html.hbs Outdated Show resolved Hide resolved
@oshi97
Copy link
Contributor

oshi97 commented May 4, 2022

I would probably remove the "fix" part from the title of the PR? This feels more like a feature than a bugfix - imo things were working as expected and this is kind of just how iframes are

@yen-tt yen-tt changed the title Fix iFrame'd Experience Tab Navigation keep parent's domain for tab navigation on new tab in iframe experience May 4, 2022
@yen-tt yen-tt requested review from oshi97 and a team May 5, 2022 18:43
yen-tt added a commit to yext/answers-search-ui that referenced this pull request May 6, 2022
This pr is made in tandem with the [theme changes](yext/answers-hitchhiker-theme#1067) to support iframe experience's tab navigation where opening one in a new browser tab should still be under the domain of the iFrame's parent for the new page's url.

if a `_parentUrl` is provided, the tabs are updated with their href having the domain of the parent url and a verticalUrl param attached to indicate which page to navigate to. The target is also updated to '_parent' to handle the case of tab navigation in the same browser tab.
a function `setParentUrl` is added to the navigation component for user to set `_parentUrl`, which will cause a rerender of the component to update the tabs's link


J=SLAP-2089
TEST=manual

Tested along with the changes in theme. 
Deployed two test sites in storm, with [one being from the theme dev branch](https://devtabnaviniframe-theme-slapshot-pagescdn-com.preview.pagescdn.com/?query=virginia&referrerPageUrl=) that points to the SDK pr dev branch, under the staging commit: dev/tab-nav-in-iframe[@65f09b10](yext/answers-hitchhiker-theme@65f09b1). That is used in the src script of iframe_test.html of the [other site](https://devyttesting-theme-slapshot-pagescdn-com.preview.pagescdn.com/?query=virginia&referrerPageUrl=) under the staging commit: dev/yt-testing[@517e852f](yext/answers-hitchhiker-theme@517e852). Navigate to different tabs in the same tab and opening them in new tab. See that the page is still under the domain of the iFrame's parent.
@yen-tt yen-tt merged commit de6108a into develop May 6, 2022
@yen-tt yen-tt deleted the dev/tab-nav-in-iframe branch May 6, 2022 18:14
@nmanu1 nmanu1 mentioned this pull request May 10, 2022
nmanu1 added a commit that referenced this pull request May 10, 2022
## Version 1.29.0
### Features
- Added support for passing Answers Agents in the HTTP header (#1056)
- Added support for adding custom autocomplete prompts (#1040)
- Added RTF truncation support (#1060, #1061)

### Changes
- Updated default `vertical-grid` universal limit to 3 (#1048)

### Bug Fixes
- Fixed filters and footer overlap on desktop view (#806)
- Fixed `CollapsibleFilters` tab order (#1057)
- Fixed text truncation in the accordion card (#1065)
- Updated iframe tab navigation to keep parent’s domain (#1067)
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