fix(embedded-sdk): add accessible title to iframe #27017
Conversation
superset-embedded-sdk/src/index.ts
Outdated
| @@ -154,6 +154,7 @@ export async function embedDashboard({ | |||
| }); | |||
|
|
|||
| iframe.src = `${supersetDomain}/embedded/${id}${dashboardConfig}${filterConfigUrlParams}`; | |||
| iframe.title = 'Embedded Dashboard'; | |||
There was a problem hiding this comment.
This works in a pinch, but does the dashboard config (or something else) happen to provide the actual title of the dashboard? That would be the ideal thing to put here.
There was a problem hiding this comment.
No not that I'm aware of, and the SDK doesn't fetch it either. It looks like the SDK doesn't load anything from Superset API at all — just gets a token using the opaque function it's given, and passes it to the iframe content.
There was a problem hiding this comment.
It seems the sdk should expose the option to set this as maybe an iframeTitle property of the dashboardConfig object (?), and fall back on some default like this one "Embedded Dashboard". Note that for extra points you could i18n that string as well.
There was a problem hiding this comment.
Moved it to the config. Didn't provide a fallback for now because I'd probably butcher the i18n. Would a fallback be required for this, or okay without?
There was a problem hiding this comment.
Ok without AFAIC, the i18n is easy though, just FYI ->
import { t } from '@superset-ui/core';
const translatableString = t("Embedded Dashboard");What happens then is the i18n framework scans through the code base and inventorize all those strings on the backend and frontend, where other people can then translate then in various languages.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Thanks for following up... running CI 🤞 |
|
I believe the whole ui/core package would need to be added as a dependency here. I haven't tried to open the can of worms of actually running this repo so not confident in being able to get that working. |
superset-embedded-sdk/src/index.ts
Outdated
| @@ -77,6 +80,7 @@ export async function embedDashboard({ | |||
| id, | |||
| supersetDomain, | |||
| mountPoint, | |||
| iframeTitle = t("Embedded Dashboard"), | |||
There was a problem hiding this comment.
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!
superset-embedded-sdk/src/index.ts
Outdated
| @@ -154,6 +158,9 @@ export async function embedDashboard({ | |||
| }); | |||
|
|
|||
| iframe.src = `${supersetDomain}/embedded/${id}${dashboardConfig}${filterConfigUrlParams}`; | |||
| if(iframeTitle) { | |||
There was a problem hiding this comment.
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 :/
Oh you're right about that! Let's actually not do this then. Not worth adding the dependency for the sake of a i18n that one string! |
|
@mistercrunch @bhaugeea Curious if you see a way forward with this, so we can close out the PR and Issue. If there's no intent to rebase this in the near future and get it across the finish line, I'll convert this to a Draft, and the issue may closed as stale in time. |
Addressing my comments
|
@rusackas I just made the changes I suggested and removed |
|
I don't see this change is published on npmjs registry with version 0.1.0-alpha.11. How can we get the new package with this change? |
|
Sorry... we need to update a bunch of npm packages. We'll try to get that effort underway soon. |
|
@rusackas is the process to do that documented someplace? Oh nevermind found an example PR -> https://github.com/apache/superset/pull/13593/files |
|
That brings in the newly published ones, but here are the instructions (the most current I'm aware of) to actually publish the modules to NPM: https://github.com/apache/superset/pull/23730/files This should be made part of the release process, but for now, I have it on my task list to go back and try it on some of the voted-in release branches/SHAs. |
Co-authored-by: Maxime Beauchemin <maximebeauchemin@gmail.com>
SUMMARY
Screen reader users may rely on the (missing) title attribute to know that the iframe is where the dashboard content is. This PR adds a title to the iframe with the generic value "Embedded Dashboard".
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION