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

(components): Render extra Embed props on iframe tag #1122

Merged
merged 6 commits into from
Dec 14, 2020

Conversation

lachlanjc
Copy link
Member

Closes #966, this PR makes extra props on the Embed component, especially/such as title or aria-label, be rendered on the iframe tag instead of the container div, to allow better accessibility when using that component.

This is a very slightly potentially breaking change for developers who are using container style props (mb={3}) directly on Embed, since those could now behave differently inside another div, but I don’t think that’s a common usage. Added a quick note about accessibility to the Embed docs as well.

Includes tests, snapshots, & docs.

@lachlanjc lachlanjc changed the title (@theme-ui/components): Render extra Embed props on iframe tag (components): Render extra Embed props on iframe tag Nov 1, 2020
Copy link
Member

@hasparus hasparus left a comment

Choose a reason for hiding this comment

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

Love it.

@hasparus
Copy link
Member

Okay, one thing that's left before releasing this is updating the changelog. This could be breaking, right?

@hasparus
Copy link
Member

hasparus commented Nov 23, 2020

The types for Embed also change, since it no longer supports box props (space, color, variant).

export interface EmbedProps
extends Assign<
React.ComponentProps<'div'> & React.RefAttributes<HTMLIFrameElement>,
BoxOwnProps
> {
ratio?: number
src?: React.IframeHTMLAttributes<any>['src']
frameBorder?: React.IframeHTMLAttributes<any>['frameBorder']
allowFullScreen?: React.IframeHTMLAttributes<any>['allowFullScreen']
allow?: React.IframeHTMLAttributes<any>['allow']
}
/**
* Responsive iframe for video embeds.
*
* Embed variants can be defined anywhere in the theme object.
*
* @see https://theme-ui.com/components/embed
*/
export const Embed: ForwardRef<HTMLIFrameElement, EmbedProps>
export interface AspectRatioProps extends BoxProps {
ratio?: number
}
/**
* Component for maintaining a fluid-width aspect ratio
* @see https://theme-ui.com/components/aspect-ratio
*/
export const AspectRatio: ForwardRef<HTMLDivElement, AspectRatioProps>
export interface AspectImageProps extends ImageProps {
ratio?: number
}
/**
* Image component constrained by as aspect ratio.
* @see https://theme-ui.com/components/aspect-image
*/
export const AspectImage: ForwardRef<HTMLImageElement, AspectImageProps>

I think we can get rid of space and color props, but pass variant to the root child.

@hasparus
Copy link
Member

Hmm... maybe we could still spread on Box, but pick a greater set of iframe props to pass on to the child iframe?

@hasparus hasparus self-requested a review November 23, 2020 09:02
@lachlanjc
Copy link
Member Author

Yep, I realize I've forgotten to update the changelog with several recent PRs (like print color mode support). I will add that. Sounds good with variant too!

@lachlanjc lachlanjc added the enhancement New feature or request label Dec 3, 2020
@lachlanjc lachlanjc self-assigned this Dec 6, 2020
@hasparus hasparus closed this Dec 14, 2020
@hasparus hasparus deleted the branch system-ui:develop December 14, 2020 08:53
@hasparus hasparus reopened this Dec 14, 2020
@vercel
Copy link

vercel bot commented Dec 14, 2020

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/systemui/theme-ui/g0bix1sqf
✅ Preview: https://theme-ui-git-fix-966.systemui.vercel.app

@hasparus hasparus merged commit 349d6eb into system-ui:develop Dec 14, 2020
@lachlanjc lachlanjc deleted the fix-966 branch December 14, 2020 15:45
@lachlanjc
Copy link
Member Author

This actually wasn't quite ready to merge after our discussion @hasparus—we can use prop filtering to make using space props keep working here without a breaking change, but that can be a separate PR before we release stable

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Embed additional props for accessibility and others
2 participants