superset icon indicating copy to clipboard operation
superset copied to clipboard

Published 20 hours ago • verified publisher icon apache

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

Open bhaugeea opened this issue 1 year ago • 3 comments

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

image

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

  • [x] Has associated issue: Fixes #27016
  • [ ] Required feature flags:
  • [ ] Changes UI
  • [ ] Includes DB Migration (follow approval process in SIP-59)
    • [ ] Migration is atomic, supports rollback & is backwards-compatible
    • [ ] Confirm DB migration upgrade and downgrade tested
    • [ ] Runtime estimates and downtime expectations provided
  • [ ] Introduces new feature or API
  • [ ] Removes existing feature or API

bhaugeea avatar Feb 05 '24 18:02 bhaugeea

Thanks for following up... running CI 🤞

rusackas avatar Feb 06 '24 20:02 rusackas

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.

bhaugeea avatar Feb 06 '24 23:02 bhaugeea

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.

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 avatar Feb 07 '24 22:02 mistercrunch

@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.

rusackas avatar Jun 03 '24 16:06 rusackas

@rusackas I just made the changes I suggested and removed superset-ui/core dependency that was reference, but wasn't added as a dependancy of the sdk since it could have implications.

mistercrunch avatar Jun 03 '24 20:06 mistercrunch

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?

nikola-arcadis avatar Jun 21 '24 06:06 nikola-arcadis

Sorry... we need to update a bunch of npm packages. We'll try to get that effort underway soon.

rusackas avatar Jun 26 '24 18:06 rusackas

@rusackas is the process to do that documented someplace? Oh nevermind found an example PR -> https://github.com/apache/superset/pull/13593/files

mistercrunch avatar Jun 27 '24 00:06 mistercrunch

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.

rusackas avatar Jun 28 '24 18:06 rusackas