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

Calculate share button padding dynamically for scroll affordance #12127

Merged
merged 3 commits into from Dec 9, 2017

Conversation

alanorozco
Copy link
Member

@alanorozco alanorozco commented Nov 20, 2017

Fixes #11898.

When the share buttons won't fit in the container, thus making it scrollable, we need a visual affordance so that the user realizes that the element is scrollable.

This change allows the widget to calculate the padding so that there's always an element cut-off if needed, otherwise allows the buttons to have "fluid" padding so that they fit the container.

scrollafordance

(This could be done with CSS/flexbox IFF the element wasn't scrollable.)

@dreamofabear dreamofabear self-requested a review December 6, 2017 19:57
Copy link

@dreamofabear dreamofabear left a comment

Choose a reason for hiding this comment

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

Cool! Love the GIFs in the PR descriptions.

this.root_./*OK*/offsetLeft;

const iconWidth =
dev().assert(items[0].firstElementChild)./*OK*/offsetWidth;

Choose a reason for hiding this comment

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

Nit: Extract items[0].firstElementChild into a local var.

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.

const iconWidth =
dev().assert(items[0].firstElementChild)./*OK*/offsetWidth;

// Total width that the buttons will occupy with minimum padding

Choose a reason for hiding this comment

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

Super-nit: Trailing period.

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.

@alanorozco alanorozco merged commit e7be39a into ampproject:master Dec 9, 2017
@alanorozco alanorozco deleted the bookend-share-scroll branch December 9, 2017 00:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Stories - By Type
  
Awaiting triage
Development

Successfully merging this pull request may close these issues.

None yet

4 participants