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.

Sign up for GitHub

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

Jump to bottom

fix(embedded-sdk): add accessible title to iframe #27017

Merged
merged 6 commits into from
Jun 4, 2024
Merged

fix(embedded-sdk): add accessible title to iframe #27017

Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
765e809
Add title to embedded dashboard iframe
bhaugeea Feb 5, 2024
7a41939
move iframe title to dashboard config
bhaugeea Feb 6, 2024
fed7303
Provide default iframe title with i18n
bhaugeea Feb 6, 2024
dee2eee
Update index.ts
mistercrunch Jun 3, 2024
7de23d2
Merge branch 'master' into patch-1
mistercrunch Jun 3, 2024
fdefa2b
Update index.ts
mistercrunch Jun 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions superset-embedded-sdk/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import {
DASHBOARD_UI_FILTER_CONFIG_URL_PARAM_KEY,
IFRAME_COMMS_MESSAGE_TYPE
} from './const';
import { t } from '@superset-ui/core';

// We can swap this out for the actual switchboard package once it gets published
import { Switchboard } from '@superset-ui/switchboard';
Expand Down Expand Up @@ -51,6 +52,8 @@ export type EmbedDashboardParams = {
supersetDomain: string
/** The html element within which to mount the iframe */
mountPoint: HTMLElement
/** The iframe title attribute */
iframeTitle?: string
/** A function to fetch a guest token from the Host App's backend server */
fetchGuestToken: GuestTokenFetchFn
/** The dashboard UI config: hideTitle, hideTab, hideChartControls, filters.visible, filters.expanded **/
Expand All @@ -77,6 +80,7 @@ export async function embedDashboard({
id,
supersetDomain,
mountPoint,
iframeTitle = t("Embedded Dashboard"),
Copy link
Member

Choose a reason for hiding this comment

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

Hey this look like a public interface, so this new [optional] param should be the last one in the function call for backwards compatibility, otherwise we'l have off-by-one param matching on the function call!

fetchGuestToken,
dashboardUiConfig,
debug = false
Expand Down Expand Up @@ -154,6 +158,9 @@ export async function embedDashboard({
});

iframe.src = `${supersetDomain}/embedded/${id}${dashboardConfig}${filterConfigUrlParams}`;
if(iframeTitle) {
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 given the default value set in the function header, this will always have a value, so no if required. Actually I think if someone actually wanted an empty string and pass iframeTitle = '' or iframeTitle = null in the function call, it would prevent them from overriding the Embedded Dashboard default :/

iframe.title = iframeTitle;
}
//@ts-ignore
mountPoint.replaceChildren(iframe);
log('placed the iframe')
Expand Down
Loading