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

Feature/3942 embed project #3946

Conversation

chrisgarrity
Copy link
Contributor

Resolves

Proposed Changes

  • Adds new embed mode
  • Does not support autoplay as browser requires user action for audio to work properly in all cases
  • replaces fullscreen toggle with the Scratch Logo (linked to scratch.mit.edu)
  • Full screen toggle did nothing in the 2.0 embed. If we do want to support a full screen mode, we can revisit it later.

Will be going over styles with Kathy to see if there are any adjustments (logo size etc)

Reason for Changes

To be compatible with 2.0

Test Coverage

Manual Testing:

  • There should be no effect for anyone using Gui standalone as there's no UI to switch to embed view. (perhaps it would be a good idea to add an embed view to the playground)
  • Testing in the context of www embed URL

Browser Coverage

Check the OS/browser combinations tested (At least 2)

Mac

  • Chrome
  • Firefox
  • Safari

Windows

  • Chrome
  • Firefox
  • Edge

Chromebook

  • Chrome

iPad

  • Safari

Android Tablet

  • Chrome

paulkaplan
paulkaplan previously approved these changes Dec 5, 2018
Copy link
Contributor

@paulkaplan paulkaplan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code LG. I think we should add a playground file for this mode, and cover it with an integration test. I'm fine to file that/follow up with it after to allow this to get in / start getting tested. I can try to make one right now also.

<bikeshedding>
I am somewhat concerned about naming here, since we often refer to the GUI as being "embedded" inside the project page, and this is an unrelated version of such embedding. However, this more closely aligns with the user-facing terminology, so ¯\(ツ)/¯ Since the only thing the actual redux state controls is whether the scratch logo is shown instead of the fullscreen buttons, I think it would also be fine to call it "show branding" or similar (the size and player-only-ness is controlled by the isPlayerOnly, which I think is good). I'm ok with keeping the exported initializer initEmbedded, since that sets several relevant mode flags.
</bikeshedding>

Liked @paulkaplan’s suggestion that the property get renamed `showBranding` as we use ‘embedded’ in multiple ways that would get confusing.

The embedded project view is ‘playerOnly’, ‘fullscreen’ and ‘showBranding’.
@chrisgarrity
Copy link
Contributor Author

@paulkaplan I liked the showBranding suggestion, so I did that, and opened #3958 to follow up with a playground view.

@chrisgarrity chrisgarrity merged commit c05a936 into scratchfoundation:develop Dec 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants