-
-
Notifications
You must be signed in to change notification settings - Fork 8.8k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
fix(create): update broken SVG paths in templates #6762
Conversation
Hi @anicholls! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks! |
✔️ [V2] 🔨 Explore the source changes: a51dbac 🔍 Inspect the deploy log: https://app.netlify.com/sites/docusaurus-2/deploys/621997202a87f10007ce303f 😎 Browse the preview: https://deploy-preview-6762--docusaurus-2.netlify.app |
⚡️ Lighthouse report for the changes in this PR:
Lighthouse ran on https://deploy-preview-6762--docusaurus-2.netlify.app/ |
@anicholls why did you close this? I am also having the same issue 😄 |
Yeah, no need to close—the CLA status gets updated automatically |
PR checks out; the patch solved the issue for me! |
We should probably have a TypeScript E2E test as well... This template is currently untested |
Sorry! I found another, related issue that I wanted to address in the PR that I wasn't able to get to today. I'm unavailable for a few days - didn't want to leave it hanging and have it merged before I had a chance to provide an update.
This scenario also needs a different approach because the images are SVGs (which seem to be treated differently than PNGs).
I will get back to it next week unless someone beats me to it :)
Get Outlook for Android<https://aka.ms/AAb9ysg>
…________________________________
From: Joshua Chen ***@***.***>
Sent: Friday, February 25, 2022 5:44:07 PM
To: facebook/docusaurus ***@***.***>
Cc: Alex Nicholls ***@***.***>; Mention ***@***.***>
Subject: [External Sender] Re: [facebook/docusaurus] Update broken image URLs in typescript template example (PR #6762)
Yeah, no need to close—the CLA status gets updated automatically
—
Reply to this email directly, view it on GitHub<#6762 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AAG5ZPXZO3BLAXCODQVBP2DU5AV6PANCNFSM5PK3GY7A>.
Triage notifications on the go with GitHub Mobile for iOS<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
I will directly update this PR. Did you get to sign the CLA? As long as you sign it, I will be able to merge your fix. I'd prefer if you can sign the CLA this weekend, because it's likely we do a hotfix release at the start of next week. |
return ( | ||
<div className={clsx('col col--4')}> | ||
<div className="text--center"> | ||
<img className={styles.featureSvg} alt={title} src={image} /> | ||
<Svg className={styles.featureSvg} role="img" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a side fix, because TypeScript correctly warns us that svg
elements should not have alt
. Instead, I've added a <title>
to each SVG graphic. See also #6684
@anicholls Ping again, would you be able to sign the CLA this weekend? I hope to get this PR merged soon so I can add TS to the test matrix. |
@Josh-Cena done! Thanks for jumping on this so quickly 🙏 |
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
Thanks! |
Motivation
I recently tried a simple out-of-the-box setup of Docusaurus using the following script:
This installed cleanly, but after starting or building the site I got the following errors:
After inspection, it appeared that an unnecessary
src/
had been put in the image src. Thestatic/
folder in this example is in the root, not insrc/
, so removing this fixed the errors.