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

Crowdin export formatting fixes #4806

Merged
merged 5 commits into from Dec 17, 2021
Merged

Crowdin export formatting fixes #4806

merged 5 commits into from Dec 17, 2021

Conversation

minimalsm
Copy link
Contributor

Description

Changes

  1. Wraps iframe videos in a figure element
  2. Forces all ButtonLinks to span multiple lines

Reasoning

  1. By wrapping an iframe in a block element we prevent an issue we were having where our translation service Crowdin would rebuild the page incorrectly (treating the iframe and the previous image as one block of text) when it was exported.

How we upload it:
image

How Crowdin rebuilds it:
image

  1. The ButtonLink has the same bug where it was forced onto one block of text with any previous text. By spanning the ButtonLink over multiple lines we force Crowdin to render a block-level element instead of an inline element.

Usage examples

<!--- Good -->
<ButtonLink to="/eth2/merge/">
  The merge
</ButtonLink>

<!--- Bad -->
<ButtonLink to="/eth2/merge/">The merge</ButtonLink>
<!--- Good -->
Vitalik Buterin, when talking to Bankless podcast, presented 3 potential options that are worth discussing.

<figure>
  <iframe width="100%" height="315" src="https://www.youtube.com/embed/-R0j5AMUSzA?start=5841" frameborder="0" allow="accelerometer; autoplay; clipboard-write; encrypted-media; gyroscope; picture-in-picture" allowfullscreen></iframe>
</figure>

<!--- Bad -->

Vitalik Buterin, when talking to Bankless podcast, presented 3 potential options that are worth discussing.

<iframe width="100%" height="315" src="https://www.youtube.com/embed/-R0j5AMUSzA?start=5841" frameborder="0" allow="accelerometer; autoplay; clipboard-write; encrypted-media; gyroscope; picture-in-picture" allowfullscreen></iframe>

Related Issue

None

@minimalsm minimalsm added the refactor ♻️ Changes which don't affect functionality label Dec 16, 2021
@github-actions github-actions bot added content 🖋️ This involves copy additions or edits review needed 👀 labels Dec 16, 2021
src/content/defi/index.md Outdated Show resolved Hide resolved
@gatsby-cloud
Copy link

gatsby-cloud bot commented Dec 17, 2021

Gatsby Cloud Build Report

ethereum-org-website-dev

🎉 Your build was successful! See the Deploy preview here.

Build Details

View the build logs here.

🕐 Build time: 8m

Performance

Lighthouse report

Metric Score
Performance 🔶 21
Accessibility 💚 97
Best Practices 💚 93
SEO 🔶 79

🔗 View full report

@wackerow
Copy link
Member

https://www.gatsbyjs.com/dashboard/5e02f963-3fe5-4e37-a1e7-ac05f7753a86/sites/dc7c4340-a8e9-44ed-b7be-883c70d2f02d/builds/6636792c-b872-4477-a9a5-c04879b467c4/details
Build passed after clearing cache on Gatsby side and rebuilding. Not sure why it didn't update here, but going to bring this in.

This is making me more in favor of just making a YouTube component that can be used more easily throughout the side. Very easy to make a mistake with the current setup, and we have pretty much standardized how we embed YouTube videos anyway. Can address this separately.

@wackerow wackerow merged commit 33fbaca into dev Dec 17, 2021
@wackerow wackerow deleted the crowdInFixes branch December 17, 2021 00:30
@wackerow wackerow mentioned this pull request Dec 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
content 🖋️ This involves copy additions or edits refactor ♻️ Changes which don't affect functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants