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

Force set amp-lightbox-gallery buttons to content-box sizing #15080

Merged
merged 3 commits into from May 7, 2018

Conversation

cathyxz
Copy link
Contributor

@cathyxz cathyxz commented May 4, 2018

Force buttons to be content-size, since sometimes people will do

* {
   box-sizing: border-box;
}

As a pattern.

Copy link
Contributor

@cvializ cvializ left a comment

Choose a reason for hiding this comment

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

Our build has an svg optimizer pass, and it does this conversion automatically:

// build/amp-lightbox-gallery-0.1.css.js
export const CSS = "/*...*/url(\"data:image/svg+xml;charset=utf-8,%3Csvg viewBox='0 0 24 24' xmlns='http://www.w3.org/2000/svg'%3E%3Cpath d='M18.295 5.705a.997.997 0 0 0-1.41 0L12 10.59 7.115 5.705a.997.997 0 1 0-1.41 1.41L10.59 12l-4.885 4.885a.997.997 0 0 0 1.41 1.41L12 13.41l4.885 4.885a.997.997 0 0 0 1.41-1.41L13.41 12l4.885-4.885a.997.997 0 0 0 0-1.41z' fill='%23fff' fill-rule='nonzero'/%3E%3C/svg%3E\"/*...*/";

@cathyxz cathyxz changed the title lightbox svg fixes Force set amp-lightbox-gallery buttons to content-size May 4, 2018
@cathyxz cathyxz changed the title Force set amp-lightbox-gallery buttons to content-size Force set amp-lightbox-gallery buttons to content-box sizing May 4, 2018
Copy link
Contributor

@cvializ cvializ left a comment

Choose a reason for hiding this comment

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

LGTM

Side note: I noticed that the button with this classname is a div tag. button tags do have some default styles to override (like background image and border), but they also default to box-sizing: border-box.

@@ -87,6 +87,7 @@ amp-lightbox-gallery .amp-carousel-button {
width: 24px;
height: 24px;
padding: 16px;
box-sizing: content-box;
Copy link
Contributor

Choose a reason for hiding this comment

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

put !important ? to protect when we actually make this public in future

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wasn't sure about this, since the intention here is to allow them to edit the dimensions of the button. There seemed to be an argument to allow them to style this however they please.

Copy link
Contributor

Choose a reason for hiding this comment

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

fair point. LGTM

@cathyxz cathyxz merged commit 1e3ccd6 into ampproject:master May 7, 2018
@cathyxz cathyxz deleted the bugfix/lightbox branch May 7, 2018 17:10
noranazmy pushed a commit to noranazmy/amphtml that referenced this pull request May 10, 2018
…ect#15080)

* Replace double quotes with single quotes for firefox windows parsing

* Set box sizing to content box

* Revert "Replace double quotes with single quotes for firefox windows parsing"

This reverts commit ade746c.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants