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

✨ [Amp story player] [Desktop panels player] Experiment setup and basic styling #34956

Merged
merged 26 commits into from Jul 26, 2021

Conversation

processprocess
Copy link
Contributor

@processprocess processprocess commented Jun 22, 2021

Context / Fixes #34953

  • Sets a boolean variable on window to toggle experiment from unit tests.
  • Adds next and previous story buttons.
  • Sets up new classes and basic styling based on the desktop-one-panel variables.

@amp-owners-bot
Copy link

amp-owners-bot bot commented Jun 22, 2021

Hey @gmajoulet, @newmuis! These files were changed:

extensions/amp-story/1.0/amp-story-desktop-one-panel.css
extensions/amp-story/1.0/amp-story.js

@processprocess processprocess requested review from rileyajones and removed request for rileyajones June 22, 2021 14:38
@processprocess processprocess removed the request for review from rileyajones July 12, 2021 14:44
@processprocess processprocess marked this pull request as draft July 13, 2021 20:23
@processprocess processprocess marked this pull request as ready for review July 14, 2021 16:23
extensions/amp-story/1.0/amp-story.js Show resolved Hide resolved
src/amp-story-player/amp-story-player-impl.js Outdated Show resolved Hide resolved
src/amp-story-player/amp-story-player-impl.js Outdated Show resolved Hide resolved
src/amp-story-player/amp-story-player-impl.js Outdated Show resolved Hide resolved
Copy link
Contributor

@gmajoulet gmajoulet left a comment

Choose a reason for hiding this comment

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

Please make sure that the buttons are either usable or totally hidden for a publisher who's not importing our stylesheet

src/amp-story-player/amp-story-player-impl.js Outdated Show resolved Hide resolved
@processprocess
Copy link
Contributor Author

processprocess commented Jul 22, 2021

Please make sure that the buttons are either usable or totally hidden for a publisher who's not importing our stylesheet

As per discussion: The css for the buttons are in a file that's injected into the shadow dom so this should be okay.

@processprocess
Copy link
Contributor Author

cc @rileyajones for OWNERS (Renamed a CSS file in this PR).

css/amp-story-player-shadow.css Outdated Show resolved Hide resolved
css/amp-story-player-shadow.css Show resolved Hide resolved
@@ -17,7 +17,7 @@
import * as ampToolboxCacheUrl from '@ampproject/toolbox-cache-url';
import {Messaging} from '@ampproject/viewer-messaging';

// Source for this constant is css/amp-story-player-iframe.css
// Source for this constant is css/amp-story-player-shadow.css
Copy link
Contributor

Choose a reason for hiding this comment

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

Codecov would really like to see some additional tests for AmpStoryPlayer.

examples/amp-story/player-amp-desktop-panels.html Outdated Show resolved Hide resolved
processprocess and others added 2 commits July 23, 2021 14:26
Co-authored-by: Riley Jones <78179109+rileyajones@users.noreply.github.com>
Co-authored-by: Riley Jones <78179109+rileyajones@users.noreply.github.com>
@processprocess processprocess merged commit df315b6 into ampproject:main Jul 26, 2021
@processprocess processprocess deleted the player branch July 26, 2021 15:15
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.

[Amp story player] [Desktop panels player] Experiment setup
4 participants