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

Aspect Ratio component #436

Closed
claviska opened this issue Apr 26, 2021 · 4 comments
Closed

Aspect Ratio component #436

claviska opened this issue Apr 26, 2021 · 4 comments
Labels
feature Feature requests.

Comments

@claviska
Copy link
Member

Per this discussion, it may make sense to rename <sl-responsive-embed> to the more generic <sl-aspect-ratio> and modify it to support:

  • embeds (embed, iframe, object)
  • images (img, picture, figure)
  • arbitrary containers (div, etc.)

The logic will be the same for all use cases, but renaming will be a breaking change. It may be helpful to remember that this is a stop gap utility until browsers fully support aspect-ratio, after which this component can be deprecated.

Any concerns or thoughts before making this update?

/cc @ParamagicDev

@claviska claviska added the feature Feature requests. label Apr 26, 2021
@KonnorRogers
Copy link
Collaborator

No concerns here. I dont know how widely used sl-responsive-embed is since its a tightly scoped utility that most people rarely use.

@claviska
Copy link
Member Author

claviska commented May 4, 2021

Looks like we're not the first to take a more generic approach to this: https://radix-ui.com/primitives/docs/utilities/aspect-ratio

@KonnorRogers
Copy link
Collaborator

@claviska
Copy link
Member Author

claviska commented May 4, 2021

After more thought, I don't like generalizing this into <sl-aspect-ratio> because of limitations of the :slotted selector and the many, many ways users will try to slot in and position arbitrary content. This would drastically change the scope of the component, and I'm not convinced the potential use cases are worth the effort and bloat required to support it.

That said, it seems like images, videos, and embeds are the primary use case here, so the original request has been implemented as <sl-responsive-media> with a focus on replaced elements.

This component replaces <sl-responsive-embed> and introduced a breaking change, but the API is otherwise the same so simply renaming the tag will work fine for anyone using the previous one. I've also moved it under "Utilities" in the docs.

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

No branches or pull requests

2 participants