fix: enable OpenProcessing sketch embeds on detail pages - #1310
fix: enable OpenProcessing sketch embeds on detail pages#1310rushdarshan wants to merge 1 commit into
Conversation
|
Hi @rushdarshan thank you for this PR. I noticed a few things while reviewing: This PR contains two separate fixes. The reference link fix for issue #674 is already addressed in your separate PR #1284. Including it here creates duplication. cc @ksen0 @doradocodes to advise |
|
Thanks for the work @rushdarshan and the review @Nwakaego-Ego ! I've left a review on the other PR; if that one gets merged first, then the commit history of this wil also be cleaned up after updating the branch. But I would suggest in the future to work in separate local branches, or on one PR at a time, so that there's no extra commits. If you're still interested in working on it, please complete the work on the other fix PR. The OpenProcessing loading problems I think have a different root cause, but it's really hard to test and verify. Still, the proposed fix here is helpful, but because of the extra commits it must be reviewed after the other open PR is done. (Or you can close this one and reopen with a cleaned-up history without extra commits.) Thanks for your time and patience. |
7843910 to
2af8d65
Compare
|
Hi @rushdarshan, thank you for working on this. I tested the three sketch pages you shared from issue #1307, and they are all loading correctly on the live site without this PR being merged, which may suggest the root cause was on OpenProcessing's side rather than the website code. I stand to be corrected on this. The sandbox attribute is still a valid and worthwhile addition as good practice. However, there are a few concerns: The original issue only asked for the sketches to render. Adding allow="fullscreen; clipboard-write" and the error fallback UI goes beyond that scope. These are feature decisions that affect how the site behaves and should be discussed and approved through a separate issue before being added. Also, comparing your version with the original ScalingIframe component on main, the following are missing from your iframe: left: 0 These are what make the scaling work. cc @doradocodes @ksen0, kindly advise. |
Description
OpenProcessing sketch embeds on sketch detail pages were failing to render due to missing iframe security/permission attributes.
Root Cause
iframe elements lacked required `sandbox`/`allow` attributes, which could block embed scripts.
Solution
Resolves #1307