Embed Share Width #41431
Embed Share Width #41431
Conversation
| @@ -210,7 +210,8 @@ class ShareAllowedDialog extends React.Component { | |||
| embedOptions = { | |||
| // If you change this width and height, make sure to update the | |||
| // #visualizationColumn.wireframeShare css | |||
| iframeHeight: applabConstants.APP_HEIGHT + 140, | |||
| // Extra 40 pixels added to account for left and right padding divs (20 px each side) | |||
| iframeHeight: applabConstants.APP_HEIGHT + 140 + 40, | |||
There was a problem hiding this comment.
maybe i'm missing something, but why are we changing iframeHeight instead of iframeWidth here?
There was a problem hiding this comment.
also, there's a comment above that seems relevant:
// If you change this width and height, make sure to update the
// #visualizationColumn.wireframeShare css There was a problem hiding this comment.
Ah whoops, when refactoring, I added the pixels to the wrong height/width, so I'll need to go ahead and switch that back.
There was a problem hiding this comment.
As far as the comment, I tried to address that in the PR description: Because this is scoped to the embedded share view, I can ignore the comment that says changes in width should also be added to visualizationColumn.wireframeShare. This issue is limited to the embed share view, but not the traditional share page. The css in the visualizationColumn.wireframeShare keeps track of the width of the visualization. This embedded width calculation is used to determine how big the embedded iframe should be. So the phone visualization width stays the same, but since the embedded iframe holds the phone visualization and the spacing divs, we calculate the embedded iframe width using both the width of the phone visualization and the spacing divs. This comment is from many years ago, so there's not a lot of context on it, but this is my understanding based on some GitHub archaeology. It's hard for me to describe without showing you the different divs, so let me know if you want to hop on a video call!
There was a problem hiding this comment.
ah, sorry, i missed that detail in your PR description. i mostly understand, but can you point me to where/how the "spacing divs" are rendered? i'm wondering if we have this same problem in gamelab, but i don't quite understand where the spacing divs come into play.
There was a problem hiding this comment.
"containerLeft", for spacing:
code-dot-org/apps/src/StudioApp.js
Line 2281 in 60c1c5e
As far as whether this affects Game Lab, the answer is . . . yes? But also no. At some point along the way, the phone frame was removed from the Game Lab share visualization, but the calculated embed width (
), was never updated to remove the 32 pixels that are added to account for the frame. So, because we add the 32 pixels, the visualization is not cut off (although part of the 20px spacing div on the right is). So it's in the same situation, but didn't have the bug that AppLab did.There was a problem hiding this comment.
I'm going to update the offset for GameLab to be 4o pxls so that it's clear how that lines up with the AppLab one.
After fighting with relative and fixed positioning in CSS for this PR chain: #41136, I decided to just go with the simple solution. This means we are still using divs for spacing, but there are so many factors at play in the spacing, I think it just makes sense to let that exist (if it works, it works). This adds the 40 pixel offset to the calculation of embeded app, which takes into account the 20 pixel spacing for the left and right container. Because this is scoped to the embedded share view, I can ignore the comment that says changes in width should also be added to visualizationColumn.wireframeShare.
PR Checklist: