🖍 [Story icons] Replaced icons and styles#37122
🖍 [Story icons] Replaced icons and styles#37122mszylkowski merged 18 commits intoampproject:mainfrom
Conversation
|
Hey @gmajoulet, @newmuis! These files were changed: |
|
Can you file one (or two) tickets for the followups plz? |
| flex-direction: row !important; | ||
| justify-content: flex-end !important; | ||
| padding-top: 6px !important; | ||
| padding: 8px 4px !important; |
There was a problem hiding this comment.
Double checking that we want this updated margin on full-bleed mobile as well as desktop-one-panel.
There was a problem hiding this comment.
The thoughts on mobile is that many platforms (in apps and through AmpStoryPlayer) will want to add border radiuses to the viewers, which is why we add the margins in mobile view as well (for example the Discover feed has small radiuses now because it would hide the progress bar otherwise). We thought about being able to toggle this through a viewer message or config, but it would be more inconsistent overall, especially if viewers don't configure things properly (most of the times people don't read every sentence in the docs).
So yes, it's intended to have the added margins in mobile as well, so viewers can use larger border radiuses (which looks more modern).
| .i-amphtml-story-system-layer-buttons button[disabled] { | ||
| opacity: 0.3 !important; | ||
| cursor: initial !important; | ||
| .i-amphtml-story-system-layer-buttons button[disabled][disabled] { |
There was a problem hiding this comment.
I don't think i've seen [disabled][disabled] before.
Out of curiosity, how is this different than [disabled]?
There was a problem hiding this comment.
It's just a compact way of increasing the specificity of the selector, it doesn't technically change anything else.
| @@ -321,70 +269,42 @@ | |||
| display: block !important; | |||
| } | |||
There was a problem hiding this comment.
We might be able to remove this.
There was a problem hiding this comment.
I think this helps for keyboard users (part of the a11y efforts), that it shows the pause button for them to tab through and select, right?
… 'main' of github.com:ampproject/amphtml into replaceIcons
| } | ||
|
|
||
| .i-amphtml-story-player-panel-prev, .i-amphtml-story-player-panel-next { | ||
| background-image: url('data:image/svg+xml;charset=utf-8,<svg xmlns="http://www.w3.org/2000/svg" width="48" height="48" fill="none"><path stroke="#fff" stroke-linecap="round" stroke-width="1.9" d="M17 29.2V18.8c0-1.2 1.4-2 2.5-1.3l7.5 5.2c1 .6 1 2 0 2.6l-7.5 5.2c-1 .7-2.5 0-2.5-1.3Z"/><rect width="1.8" height="15.6" x="30.2" y="16.2" fill="#fff" rx=".9"/></svg>') !important; |
There was a problem hiding this comment.
This is the SVG that I effectively changed in this file, the rest of the changes are that I used translateY(-50%) rotate(180deg) instead of adding a new SVG for the left arrow.
All the other lines changed were just a formatting issue with the original file where there was extra indentation in most of the file :( I ran the autoformatter to fix it so it will show many lines were changed (you can hide most of the autoformatter changes with hide whitespaces in the settings menu here)
gmajoulet
left a comment
There was a problem hiding this comment.
This PR does way too much at once, it is very hard to review. Each bullet point in your description should have been a separate PR. I wouldn't be surprised if I missed one or more bugs in my review.
Contributes to #36951
First step in adopting the new UI:
current-page-has-audiofor the audio icon, can be removed in followup PR. [Story system-layer] Remove localizations strings for audio messages #37164-0.72KBSupports landscape demo
Portrait demo
Portrait:
Player:
Supports landscape: