Skip to content

🐛Adds responsive layout support to amp-soundcloud#23148

Merged
sparhami merged 7 commits intoampproject:masterfrom
wassgha:soundcloud-responsive
Jul 23, 2019
Merged

🐛Adds responsive layout support to amp-soundcloud#23148
sparhami merged 7 commits intoampproject:masterfrom
wassgha:soundcloud-responsive

Conversation

@wassgha
Copy link
Copy Markdown
Contributor

@wassgha wassgha commented Jul 2, 2019

Closes #23144

Changes

  • Makes the responsive layout a valid prop
  • Fixes some old examples with broken links

@wassgha wassgha requested a review from aghassemi July 2, 2019 05:09
@kristoferbaxter kristoferbaxter requested a review from sparhami July 2, 2019 05:41
@kristoferbaxter kristoferbaxter added Component: Layout & CSS Type: DevX issues impacting developer experience labels Jul 2, 2019
Copy link
Copy Markdown
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.

A couple of questions

@cvializ
Copy link
Copy Markdown
Contributor

cvializ commented Jul 2, 2019

Screen Shot 2019-07-02 at 12 49 58

The background image is a little cut off in the example, but I think that's polish and not necessarily a bug

Copy link
Copy Markdown
Member

@Gregable Gregable left a comment

Choose a reason for hiding this comment

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

Validator changes look good.

Copy link
Copy Markdown
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

@wassgha wassgha self-assigned this Jul 6, 2019
@wassgha wassgha requested review from Gregable and cvializ July 6, 2019 22:45
@sparhami
Copy link
Copy Markdown

sparhami commented Jul 8, 2019

I think it would be worth expanding this to use isLayoutSizeDefined:

amphtml/src/layout.js

Lines 149 to 159 in 1366241

export function isLayoutSizeDefined(layout) {
return (
layout == Layout.FIXED ||
layout == Layout.FIXED_HEIGHT ||
layout == Layout.RESPONSIVE ||
layout == Layout.FILL ||
layout == Layout.FLEX_ITEM ||
layout == Layout.FLUID ||
layout == Layout.INTRINSIC
);
}

For example, layout="fill" could be used to do responsive sizing via CSS (on a parent container) rather than through attributes. There may be cases for components where you only want one of the size defining layouts, but I think in most cases, you only care that the size is defined, not how.

Copy link
Copy Markdown

@sparhami sparhami left a comment

Choose a reason for hiding this comment

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

Left a comment, I think we should change to isLayoutSizeDefined.

@wassgha wassgha requested a review from sparhami July 20, 2019 00:01
@sparhami sparhami merged commit d51f3b8 into ampproject:master Jul 23, 2019
rindo pushed a commit to logly/amphtml that referenced this pull request Jul 24, 2019
* Added support for all size defining layouts to amp-soundcloud and fixed some examples
@Gregable Gregable mentioned this pull request Jul 25, 2019
Gregable pushed a commit that referenced this pull request Jul 25, 2019
* cl/258377914 Revision bump for #23122

* cl/258634966 Revision bump for #23349

* cl/258870451 Fix incorrect allowance of `<form method="get">` with relative URLs

* cl/259565186 Revision bump for #23386

* cl/259587993 Add a descriptive comment to amp-carousel rules.

* cl/259661215 Fix #23012 by removing the dispatch key on amp-carousel type=carousel.

* cl/259662509 Revision bump for #23148

* cl/259988931 Revision bump for #23482
@wassgha wassgha deleted the soundcloud-responsive branch August 4, 2019 22:48
@westonruter
Copy link
Copy Markdown
Member

FYI: The WordPress plugin is applying this change to have 100% visual parity in the SoundCloud embed blocks between AMP and non-AMP: ampproject/amp-wp#3003

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla: yes Component: Layout & CSS Type: DevX issues impacting developer experience

Projects

None yet

Development

Successfully merging this pull request may close these issues.

amp-soundcloud should support responsive layout

7 participants