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

Factor-out scrolling logic and styling for share widget. #12628

Merged
merged 11 commits into from Jan 10, 2018

Conversation

alanorozco
Copy link
Member

@alanorozco alanorozco commented Jan 2, 2018

Factor-out scrolling logic for share widget.

Basically to allow multiple rendering modes.

A third rendering style (wrap) is upcoming, so this simplifies things.

Also renames i-amphtml-story-share (desktop-only) to i-amphtml-story-share-pill for clarity.

@@ -321,8 +317,8 @@ amp-story-page.i-amphtml-story-page-shown {
opacity: 1!important;
}

.i-amphtml-story-share .i-amphtml-story-share-list > li + li {
margin-left: 8px!important;
.i-amphtml-story-share .i-amphtml-story-share-list > li {
Copy link
Member

Choose a reason for hiding this comment

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

can we remove rules on common tag names like li?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

.i-amphtml-story-share:hover:before {
width: calc(100% + 16px)!important;
[desktop] .i-amphtml-story-share:hover:before {
width: calc(100% - 32px) !important;
Copy link
Member

Choose a reason for hiding this comment

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

use transform: scale(0) perhaps?

Copy link
Member

Choose a reason for hiding this comment

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

probably will need some extra work with scale thing

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I was thinking to do that but it's too complicated given the rounded corners. Would basically have to have additional elements/pseudo-elements for the "pill" ends that translate, whilst a middle element scales. Tricky.

Since it was done this way at the beginning and the performance hit for animating width is negligible in this case, I'd rather leave it as it is.

@alanorozco alanorozco changed the title Split layout styles for share widget Factor-out scrolling logic and styling for share widget. Jan 9, 2018
}

.i-amphtml-story-share .i-amphtml-story-share-list > li {
.i-amphtml-story-share-pill > * > * > .i-amphtml-story-share-item {
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't > * > * > seem fragile? I wouldn't expect e.g. wrapping something in a <div> to completely break it. I don't have context on what this actually is, but maybe we can add a more specific CSS class? I don't feel strongly, but not sure if this will stand the test of time

Copy link
Member Author

Choose a reason for hiding this comment

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

Honestly I just wanted to (eagerly) optimize the selector, but we probably don't need to.

}

.i-amphtml-story-share-widget-scrollable {
padding: 16px 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

!important


.i-amphtml-story-share-widget-scrollable {
padding: 16px 0;
height: 66px;
Copy link
Contributor

Choose a reason for hiding this comment

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

!important

@alanorozco alanorozco merged commit 8e7adce into ampproject:master Jan 10, 2018
@alanorozco alanorozco deleted the sharewidget branch January 10, 2018 01:31
gzgogo pushed a commit to gzgogo/amphtml that referenced this pull request Jan 26, 2018
RanAbram pushed a commit to RanAbram/amphtml that referenced this pull request Mar 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Stories - By Type
  
Awaiting triage
Development

Successfully merging this pull request may close these issues.

None yet

4 participants