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

Refactor: Use AspectRatio in Youtube component #8445

Merged

Conversation

TylerAPfledderer
Copy link
Contributor

Description

Replaces Box component with a more robust AspectRatio component, which is primarily used for media elements. Adds enhanced sizing with aspect ratio equal, and equal existing sizing of the iframe in its current rendering.

It also creates a better render at mobile screen sizes.

Before:

Screen Shot 2022-11-01 at 23 59 56

After:

Screen Shot 2022-11-01 at 23 59 01

Note:

This makes the iframe styling in the styles file unnecessary.

Related Issue

N/A

@gatsby-cloud
Copy link

gatsby-cloud bot commented Nov 2, 2022

✅ ethereum-org-website-dev deploy preview ready

Copy link
Member

@pettinarip pettinarip left a comment

Choose a reason for hiding this comment

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

Love this.

There is one more place where we are using an iframe:
https://github.com/ethereum/ethereum-org-website/blob/dev/src/content/developers/docs/consensus-mechanisms/pos/attack-and-defense/index.md#L68

I think we should implement the AspectRatio in there as well since without those global styles it is breaking.

@TylerAPfledderer
Copy link
Contributor Author

@pettinarip I shall make a note to take a look at that as well. 👍

@TylerAPfledderer
Copy link
Contributor Author

TylerAPfledderer commented Nov 8, 2022

Love this.

There is one more place where we are using an iframe: https://github.com/ethereum/ethereum-org-website/blob/dev/src/content/developers/docs/consensus-mechanisms/pos/attack-and-defense/index.md#L68

I think we should implement the AspectRatio in there as well since without those global styles it is breaking.

@pettinarip Would you consider a more generalized AspectRatio component in place of the Youtube component?

With what the Youtube component renders and the iframe you referenced, I am seeing a pattern of styling:

  • Aspect ratio of roughly 16 / 9
  • maxWidth of 560px
  • Vertical margin of 32px (8)
  • Both could compose the figure element as the wrapper.

You would have to supply this new component with the above styling as default to override the defaults coming from the Chakra component, as AspectRatio is not a part of the theme config. In other words, you would not be able to set base styles in the extended theming.

What do you think?

Copy link
Member

@pettinarip pettinarip left a comment

Choose a reason for hiding this comment

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

@TylerAPfledderer thanks for sharing that thinking, really appreciate it.

Now that I think more about it, I think that we are good and we cover most of our use cases with this refactor and component that you did in this PR.

I don't think that we need a more generalized component for this right now unless we see more often that we use this component.

Merging it as it is for now. Good job on it!

@pettinarip pettinarip merged commit fb547cf into ethereum:dev Nov 18, 2022
@TylerAPfledderer TylerAPfledderer deleted the refactor/youtube-aspect-ratio branch November 18, 2022 22:44
@corwintines corwintines mentioned this pull request Nov 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants