Skip to content

🐛 [Amp story] Desktop one panel play button#35775

Merged
processprocess merged 3 commits intoampproject:mainfrom
processprocess:desktop-panel-play-button
Aug 23, 2021
Merged

🐛 [Amp story] Desktop one panel play button#35775
processprocess merged 3 commits intoampproject:mainfrom
processprocess:desktop-panel-play-button

Conversation

@processprocess
Copy link
Copy Markdown
Contributor

Displays the pause/play controls when in desktop panel mode.

Fixes #35640

@amp-owners-bot
Copy link
Copy Markdown

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

extensions/amp-story/1.0/amp-story-system-layer.css

@mszylkowski
Copy link
Copy Markdown
Contributor

High level question: why don't we want the new one-panel desktop to be [desktop]? Similar to #35772 where we add the layout to the list of desktop layouts.

@processprocess
Copy link
Copy Markdown
Contributor Author

High level question: why don't we want the new one-panel desktop to be [desktop]? Similar to #35772 where we add the layout to the list of desktop layouts.

#35772 had no side effects and only effected that one line. The desktop attribute will be cleaned up as much as possible once we're ready.

Adding desktop here causes visual side effects like chunkier SVGs with different margins.

@mszylkowski
Copy link
Copy Markdown
Contributor

The desktop attribute will be cleaned up as much as possible once we're ready.

Got it, so eventually we want to bring back the desktop attribute, but for now it would introduce back some of the styles we're trying to get rid of?

Copy link
Copy Markdown
Contributor

@mszylkowski mszylkowski left a comment

Choose a reason for hiding this comment

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

Do any of the other locations that use [desktop] also need this? Still LGTM since this is the only rule we know it's been affected.

@processprocess
Copy link
Copy Markdown
Contributor Author

Got it, so eventually we want to bring back the desktop attribute, but for now it would introduce back some of the styles we're trying to get rid of?

It would make more sense for our UI naming conventions to be based on aspect ratio rather than device type.
I do not think we should bring back desktop. Once we clean up all the unused desktop code I think we'll be able to scope a name change more easily and decide if we want to do that.

@processprocess
Copy link
Copy Markdown
Contributor Author

Do any of the other locations that use [desktop] also need this? Still LGTM since this is the only rule we know it's been affected.

They do not appear to and will be removed during cleanup.

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.

Pause button on story player not functional

3 participants