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.

Sign up for GitHub

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

Jump to bottom

Moving the story desktop system layer buttons. #20949

Merged
merged 4 commits into from Feb 20, 2019
Merged

Moving the story desktop system layer buttons. #20949

merged 4 commits into from Feb 20, 2019

Conversation

gmajoulet
Copy link
Contributor

  • Removing the sharing pill
  • Moving the system layer buttons to the top right corner of the screen
  • Removing the localization ID

Screenshot

Fixes #17631

@gmajoulet gmajoulet mentioned this pull request Feb 20, 2019
Closed
@Enriqe
Copy link
Contributor

Enriqe commented Feb 20, 2019 •
edited

YES

image

@Enriqe
Copy link
Contributor

Enriqe commented Feb 20, 2019 •
edited

Can you also please remove the parts where we disable the share-pill for the visual tests, since they were flaking? (#19890)

e.g.

amphtml/examples/visual-tests/amp-story/amp-story-page-attachment.html

Lines 27 to 35 in ef29b13

/*
* The share pill text does not show in the same place consistently, and
* as such causes the visual tests to be flaky. This hides the share pill
* with visibility: hidden, so as to keep other elements in the same place
* that they would otherwise be, while deflaking the tests. See #19890.
*/
.i-amphtml-story-share-pill {
visibility: hidden;
}

@gmajoulet
Copy link
Contributor Author

Oh, thanks! Done.

newmuis
newmuis approved these changes Feb 20, 2019
View reviewed changes
Copy link
Contributor

@newmuis newmuis left a comment

Choose a reason for hiding this comment

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

❤️ Love it. The "SHARE" string (a) doesn't fit in the pill, in some languages, (b) is the only string that we can't lazy load, and (c) flakes all our visual tests. Somehow, removing this button is a UX, i18n, performance, and testing improvement, all in one.

@gmajoulet gmajoulet merged commit 2277223 into ampproject:master Feb 20, 2019
@gui-poa
Copy link

gui-poa commented Mar 7, 2019

Hi, all! Is this feature on production?

Im still seeing the share string instead the icon.

@gmajoulet
Copy link
Contributor Author

This feature is part of this week's release to production, which should happen today or tomorrow.

noranazmy pushed a commit to noranazmy/amphtml that referenced this pull request Mar 22, 2019
@gmajoulet
Moving the story desktop system layer buttons. (ampproject#20949)
7d989f0 
* Removing the desktop sharing pill and moving all the buttons to the top right corner.

* Removing the AMP_STORY_SYSTEM_LAYER_SHARE_WIDGET_LABEL localization ID.

* Removing the share pill code from the visual diff tests.

* Fixing unit tests since the share menu is now always pre-rendered.
bramanudom pushed a commit to bramanudom/amphtml that referenced this pull request Mar 22, 2019
@gmajoulet @bramanudom
Moving the story desktop system layer buttons. (ampproject#20949)
1300996 
* Removing the desktop sharing pill and moving all the buttons to the top right corner.

* Removing the AMP_STORY_SYSTEM_LAYER_SHARE_WIDGET_LABEL localization ID.

* Removing the share pill code from the visual diff tests.

* Fixing unit tests since the share menu is now always pre-rendered.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@newmuis newmuis newmuis approved these changes

@Enriqe Enriqe Enriqe approved these changes

Assignees
No one assigned
Labels
cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@gmajoulet @Enriqe @gui-poa @newmuis @googlebot