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

fix: Add Support for images with absolute URLs #1033

Merged
merged 3 commits into from May 31, 2022
Merged

fix: Add Support for images with absolute URLs #1033

merged 3 commits into from May 31, 2022

Conversation

cmumatt
Copy link
Contributor

@cmumatt cmumatt commented May 30, 2022

Description

Fixes issue #1032

This PR provides support for absolute image URLs in Style programs within the IDE and Automator. When a URL is absolute, it is fetched directly and thus bypasses the normal example/roger/local/gist handlers.

Implementation strategy and design decisions

Detect and handle absolute image URLs. Only handle relative URLs using the existing logic in the IDE and Automator.

Checklist

  • I have commented my code, particularly in hard-to-understand areas
  • My changes generate no new ESLint warnings
  • I have reviewed any generated changes to the diagrams/ folder

Open questions

  • Local image fetching in the IDE is currently marked as TODO. See issue Local image resolution is not working in IDE #1035. This needs to be implemented in a separate PR.
  • Absolute image resolution in the IDE does not use a proxy to bypass CORS. If the web server hosting the image does not provide headers indicating a cross-origin request is allowed (e.g., Access-Control-Allow-Origin: *) then the browser will not fulfill the request and the placeholder image will be displayed instead.

@codecov
Copy link

codecov bot commented May 30, 2022

Codecov Report

Merging #1033 (9a0849e) into main (fbb902e) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #1033   +/-   ##
=======================================
  Coverage   64.18%   64.18%           
=======================================
  Files          62       62           
  Lines        7954     7954           
  Branches     1802     1802           
=======================================
  Hits         5105     5105           
  Misses       2733     2733           
  Partials      116      116           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fbb902e...9a0849e. Read the comment docs.

@cmumatt cmumatt linked an issue May 30, 2022 that may be closed by this pull request
@cloudflare-pages
Copy link

cloudflare-pages bot commented May 30, 2022

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 9a0849e
Status: ✅  Deploy successful!
Preview URL: https://0f5a8763.penrose-72l.pages.dev

View logs

@cmumatt cmumatt marked this pull request as ready for review May 30, 2022 22:24
@cmumatt cmumatt changed the title fix: Add Support for absolute image URLs fix: Add Support for images with absolute URLs May 30, 2022
@cmumatt cmumatt requested a review from joshsunshine May 30, 2022 22:31
@joshsunshine
Copy link
Member

@cmumatt Please add a test to the registry that includes Image shapes with href properties corresponding to various absolute URLs.

@joshsunshine
Copy link
Member

This doesn't seem to work yet with https://www.cs.cmu.edu/~jssunshi/images/ball.svg

@cmumatt
Copy link
Contributor Author

cmumatt commented May 31, 2022

@cmumatt Please add a test to the registry that includes Image shapes with href properties corresponding to various absolute URLs.

Thanks! Replaced relative paths with absolute URLs in: packages/examples/src/set-theory-domain/venn-3d.sty

@cmumatt
Copy link
Contributor Author

cmumatt commented May 31, 2022

This doesn't seem to work yet with https://www.cs.cmu.edu/~jssunshi/images/ball.svg

This is CORS. If the web server allowed the cross-origin request or we implemented a CORS proxy, then the browser would fetch the cross-origin image successfully. I've created a separate issue (#1036) to track CORS concerns.

@cmumatt cmumatt merged commit 03a9b03 into main May 31, 2022
@cmumatt cmumatt deleted the url-fun branch May 31, 2022 15:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Image URLs
2 participants