[Product Pull Request] fix: make iframe wrapper take all vieport width

[jmakowski1123]

Abstract

For Contributing Author:

This is the Primary Product Ticket for the following community contribution: Add a toggle to allow redirecting to courseware after enrollment

Checklist prior to undergoing Product Review:

The following information is required in order for Product Managers to be able to review your pull request:

• Explanation of the problem being solved
• Description of how users will be impacted, and which users will be impacted
• Screenshots or video showing the functionality or fix, before and after
• Reproduction steps and/or testing steps

Only if necessary:

• If necessary, links to corresponding configuration changes
• If necessary, links to corresponding enablement changes, particularly waffle/toggle status details

Related PRs

• feat: Add toggle to allow redirecting to courseware after enrollment. edx-platform#31715

For Product Manager doing the review:

What criteria should be analyzed from Product to approve a PR?

• The problem being solved by the feature or fix is clear.
• There is clarity on how the change or fix will impact the end user.
• It is clear that the change will not negatively impact users or other areas of the platform.
• The change is implemented comprehensively.
• Any changes to UI use the current, standard Paragon Design System: https://paragon-openedx.netlify.app/

[github-actions]

Thanks for your submission, @openedx/open-edx-project-managers will review shortly.

[jmakowski1123]

Original PR:

Resolves openedx/xblock-drag-and-drop-v2#312. Probably it was introduced in openedx/frontend-app-learning#27.

Consider how the block interface behaves in the legacy courseware (Lilac):

2023-04-02.16-46-58.mp4
On the video you can see that when the viewport reaches 480px, the block resets content width and makes it scrollable. This was implemented this way intentionally in this PR: openedx/xblock-drag-and-drop-v2#135.

Here is how it behaves in this MFE:

2023-04-02.16-49-45.mp4
Something weird happens when the viewport width reaches 574px. The iframe shrinks, making XBlock very hard to use.

With changes from this PR:

2023-04-02.16-54-47.mp4
It basically removes / compensates padding from the iframe's parent containers and sets 100% width for the iframe container itself. You can see that the block now behaves as in the legacy courseware.

Testing instructions
Switch $(DEVSTACK_ROOT)/frontend-app-learning to this branch.
Verify that xblock-drag-and-drop-v2's content behaves exactly as on the third video in different browsers.
Misc notes
You may notice this bug on the videos:
image

It's not related to this PR's changes, but possibly to this: openedx/frontend-app-learning#414

private-ref

[jmakowski1123]

@cablaa77 This one is for product review. For some reason I can't move the screenshots over from the original PR, so you can see them here. openedx/frontend-app-learning#1094

[itsjeyd]

Hi @cablaa77, could you provide a rough estimate on when we can expect product feedback for this feature?

CC @jmakowski1123

[itsjeyd]

Hi @jmakowski1123 @cablaa77 @Daniel-hershel, what can we do to get product review started for this feature?

CC:

@0x29a for openedx/frontend-app-learning#1094
@xitij2000 @mphilbrick211 for openedx/edx-platform#31715

[ali-hugo]

@itsjeyd I came across this PR and decided to look into it.

I noticed something on the final video posted on 1094. At 0:00, the course navigation fills the whole viewport width:

But, at 0:29 the navigation is narrower than the viewport width:

Is this expected behaviour? Would it be possible for the navigation to fill the entire wrap?

Other than that, the update looks great to me!

[0x29a]

@ali-hugo, this is not expected, but I think it's a separate bug. I mentioned it in the "Misc notes" section of the PR, but perhaps should have added more details:

[itsjeyd]

I came across this PR and decided to look into it.

Thanks a lot @ali-hugo!

[ali-hugo]

@ali-hugo, this is not expected, but I think it's a separate bug. I mentioned it in the "Misc notes" section of the PR, but perhaps should have added more details:

@0x29a Oh, I see. I didn't realise the icon was supposed to appear next to the navigation. Got it! Thanks for the diagram.

[antoviaque]

@ali-hugo Thank you for the review here! 👍

@jmakowski1123 Will @ali-hugo 's review be binding, ie will be sufficient to complete the product review, and allow the PR to merge once the issues fixed? Or will it still require a product review?

[itsjeyd]

(Side note for additional context: openedx/edx-platform#31715 hasn't gone through engineering review yet. So regardless of who gets to provide final/binding approval from product perspective, there will be one more step to complete before we can merge it.)

[itsjeyd]

@ali-hugo Is this good from your side now and if so, would you mind leaving an explicit approval?

Depending on @jmakowski1123's input on @antoviaque's question above, we might still need another 👍 from product, but it would be good to know if you have any additional feedback for @0x29a to address.

[itsjeyd]

@jmakowski1123 I just noticed that the info about related PRs in the issue description might need updating? In particular, the following parts seem to be referencing a different feature and PR than openedx/frontend-app-learning#1094:

For Contributing Author:

This is the Primary Product Ticket for the following community contribution: Add a toggle to allow redirecting to courseware after enrollment
Checklist prior to undergoing Product Review:

...

Related PRs

openedx/edx-platform#31715

[ali-hugo]

@ali-hugo Is this good from your side now and if so, would you mind leaving an explicit approval?

@itsjeyd @0x29a

I took another look at the videos on #1094 and everything looks good to me. You can consider this my official approval! ✅

[itsjeyd]

Great, thanks for the quick reply @ali-hugo 🎉

[itsjeyd]

Hey @jmakowski1123, when you get a minute would you mind having a look at these questions:

• [Product Pull Request] fix: make iframe wrapper take all vieport width #268 (comment)
• [Product Pull Request] fix: make iframe wrapper take all vieport width #268 (comment)

[Agrendalath]

@itsjeyd, the PR is merged now. Should we change the label and close this issue?

[itsjeyd]

@Agrendalath Updating feature tickets on the Open edX Roadmap board is currently out-of-scope for OSPR management, so I'll let @jmakowski1123 handle this part.

@jmakowski1123 Happy to ping someone else from the product team if that's what you'd prefer; just let me know.

(In the meantime I did change the label from ready for product review to product review done to avoid unnecessary churn for product reviewers looking for PRs to review.)

[sarina]

Closing as this is marked "Shipped" on the PR board 🎉