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

♻️ refactor: move builtin components to their own folders #34237

Merged
merged 4 commits into from
May 7, 2021
Merged

♻️ refactor: move builtin components to their own folders #34237

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ddee29e
refactor: move builtin components to their own folders. organization++
samouri May 5, 2021
4518cc3
fix imports / and references in md files.
samouri May 5, 2021
4e9189a
closure glob include
samouri May 6, 2021
7157608
fix dead links
samouri May 7, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion build-system/compile/sources.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,7 @@ const CLOSURE_SRC_GLOBS = [
'!third_party/babel/custom-babel-helpers.js',
// Exclude since it's not part of the runtime/extension binaries.
'!extensions/amp-access/0.1/amp-login-done.js',
'builtins/**.js',
'builtins/**/*.js',
// 'node_modules/core-js/modules/**.js',
// Not sure what these files are, but they seem to duplicate code
// one level below and confuse the compiler.
Expand Down
2 changes: 1 addition & 1 deletion build-system/test-configs/forbidden-terms.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1099,7 +1099,7 @@ const forbiddenTermsSrcInclusive = {
message: measurementApiDeprecated,
allowlist: [
'build-system/externs/amp.extern.js',
'builtins/amp-img.js',
'builtins/amp-img/amp-img.js',
'src/base-element.js',
'src/custom-element.js',
'src/iframe-helper.js',
Expand Down
10 changes: 5 additions & 5 deletions builtins/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,8 @@

The following components are always available in AMP documents:

| Component | Description |
| ----------------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
| [`amp-img`](amp-img.md) | Replacement for the HTML `img` tag. |
| [`amp-layout`](amp-layout.md) | Multi-purpose container element that brings AMP's powerful [layouts](https://amp.dev/documentation/guides-and-tutorials/develop/style_and_layout/control_layout#the-layout-attribute) to any element. |
| [`amp-pixel`](amp-pixel.md) | Used as tracking pixel to count page views. |
| Component | Description |
| ------------------------------------------ | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
| [`amp-img`](./amp-img/amp-img.md) | Replacement for the HTML `img` tag. |
| [`amp-layout`](./amp-layout/amp-layout.md) | Multi-purpose container element that brings AMP's powerful [layouts](https://amp.dev/documentation/guides-and-tutorials/develop/style_and_layout/control_layout#the-layout-attribute) to any element. |
| [`amp-pixel`](./amp-pixel/amp-pixel.md) | Used as tracking pixel to count page views. |
20 changes: 10 additions & 10 deletions builtins/amp-img.js → builtins/amp-img/amp-img.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,16 +14,16 @@
* limitations under the License.
*/

import {BaseElement} from '../src/base-element';
import {Layout, isLayoutSizeDefined} from '../src/layout';
import {ReadyState} from '../src/core/constants/ready-state';
import {Services} from '../src/services';
import {dev} from '../src/log';
import {guaranteeSrcForSrcsetUnsupportedBrowsers} from '../src/utils/img';
import {listen} from '../src/event-helper';
import {propagateObjectFitStyles, setImportantStyles} from '../src/style';
import {registerElement} from '../src/service/custom-element-registry';
import {removeElement, scopedQuerySelector} from '../src/dom';
import {BaseElement} from '../../src/base-element';
import {Layout, isLayoutSizeDefined} from '../../src/layout';
import {ReadyState} from '../../src/core/constants/ready-state';
import {Services} from '../../src/services';
import {dev} from '../../src/log';
import {guaranteeSrcForSrcsetUnsupportedBrowsers} from '../../src/utils/img';
import {listen} from '../../src/event-helper';
import {propagateObjectFitStyles, setImportantStyles} from '../../src/style';
import {registerElement} from '../../src/service/custom-element-registry';
import {removeElement, scopedQuerySelector} from '../../src/dom';

/** @const {string} */
const TAG = 'amp-img';
Expand Down
0 builtins/amp-img.md → builtins/amp-img/amp-img.md
View file
Open in desktop
File renamed without changes.
6 changes: 3 additions & 3 deletions builtins/amp-layout.js → builtins/amp-layout/amp-layout.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,9 @@
* limitations under the License.
*/

import {BaseElement} from '../src/base-element';
import {Layout, isLayoutSizeDefined} from '../src/layout';
import {registerElement} from '../src/service/custom-element-registry';
import {BaseElement} from '../../src/base-element';
import {Layout, isLayoutSizeDefined} from '../../src/layout';
import {registerElement} from '../../src/service/custom-element-registry';

class AmpLayout extends BaseElement {
/** @override @nocollapse */
Expand Down
0 builtins/amp-layout.md → builtins/amp-layout/amp-layout.md
View file
Open in desktop
File renamed without changes.
10 changes: 5 additions & 5 deletions builtins/amp-pixel.js → builtins/amp-pixel/amp-pixel.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,11 @@
* limitations under the License.
*/

import {BaseElement} from '../src/base-element';
import {Services} from '../src/services';
import {createPixel} from '../src/pixel';
import {dev, userAssert} from '../src/log';
import {registerElement} from '../src/service/custom-element-registry';
import {BaseElement} from '../../src/base-element';
import {Services} from '../../src/services';
import {createPixel} from '../../src/pixel';
import {dev, userAssert} from '../../src/log';
import {registerElement} from '../../src/service/custom-element-registry';

const TAG = 'amp-pixel';

Expand Down
0 builtins/amp-pixel.md → builtins/amp-pixel/amp-pixel.md
View file
Open in desktop
File renamed without changes.
2 changes: 1 addition & 1 deletion extensions/amp-bind/amp-bind.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1051,7 +1051,7 @@ Some AMP components and HTML elements have specific bindable attributes. They ar
- `[src]`
- `[srcset]`

Bind to `[srcset]` instead of `[src]` to support responsive images. See corresponding [`amp-img` attributes](../../builtins/amp-img.md#attributes).
Bind to `[srcset]` instead of `[src]` to support responsive images. See corresponding [`amp-img` attributes](../../builtins/amp-img/amp-img.md#attributes).
[/filter] <!-- formats="websites, ads" -->
[filter formats="email"]

Expand Down
2 changes: 1 addition & 1 deletion extensions/amp-nested-menu/amp-nested-menu.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ limitations under the License.

The `<amp-nested-menu>` component must be placed inside `<amp-sidebar>`. The component may contain the following AMP elements:

- [`<amp-img>`](../../builtins/amp-img.md)
- [`<amp-img>`](../../builtins/amp-img/amp-img.md)
- [`<amp-list>`](../amp-list/amp-list.md)
- [`<amp-accordion>`](../amp-accordion/amp-accordion.md)

Expand Down
2 changes: 1 addition & 1 deletion extensions/amp-next-page/0.1/amp-next-page.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ component will render a maximum of three documents (total) on screen at one sing

[tip type="important"]
**Important** [`<amp-analytics>`](../../amp-analytics/amp-analytics.md) is [currently unsupported](https://github.com/ampproject/amphtml/issues/15807) on pages users land on through `<amp-next-page>`.
Tracking page views is supported through [`<amp-pixel>`](../../../builtins/amp-pixel.md) or `<amp-analytics>` on the host page.
Tracking page views is supported through [`<amp-pixel>`](../../../builtins/amp-pixel/amp-pixel.md) or `<amp-analytics>` on the host page.
[/tip]

### Configuration spec
Expand Down
2 changes: 1 addition & 1 deletion extensions/amp-next-page/amp-next-page.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -341,7 +341,7 @@ Each document loads with a full-width gray horizontal line to separate it from t
The `<amp-next-page>` component supports analytics on the hosted page as well as on subsequently loaded articles. We recommend using the same analytics triggers used on standalone articles, including scroll-bound triggers.

[tip type="important"]
Tracking page views is supported through [`<amp-pixel>`](../../builtins/amp-pixel.md) or `<amp-analytics>` on the host page. It is recommended to use the `useInitialPageSize` property of `<amp-analytics>` to get a more accurate measurement of the scroll triggers otherwise the host page's `100%` trigger point would only be fired after the user scrolled past all sub-documents. Note that this will also ignore the size changes caused by other extensions (such as expanding embedded content) so some scroll events might fire prematurely instead.
Tracking page views is supported through [`<amp-pixel>`](../../builtins/amp-pixel/amp-pixel.md) or `<amp-analytics>` on the host page. It is recommended to use the `useInitialPageSize` property of `<amp-analytics>` to get a more accurate measurement of the scroll triggers otherwise the host page's `100%` trigger point would only be fired after the user scrolled past all sub-documents. Note that this will also ignore the size changes caused by other extensions (such as expanding embedded content) so some scroll events might fire prematurely instead.
[/tip]

Two custom analytics events are also provided on the host page to indicate transitioning between pages. These events can be tracked in the [amp-analytics](https://amp.dev/documentation/components/amp-analytics) config as follows:
Expand Down
2 changes: 1 addition & 1 deletion spec/amp-html-components.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,6 +171,6 @@ In these cases, services may set up endpoints that produce data that conforms to

## Components

Built-in components include [amp-img](../builtins/amp-img.md), [amp-layout](../builtins/amp-layout.md) and [amp-pixel](../builtins/amp-pixel.md).
Built-in components include [amp-img](../builtins/amp-img/amp-img.md), [amp-layout](../builtins/amp-layout/amp-layout.md) and [amp-pixel](../builtins/amp-pixel/amp-pixel.md).

AMP HTML extensions include [extended components](../extensions) and extended templates.
2 changes: 1 addition & 1 deletion spec/amp-var-substitutions.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ The following table lists the features that enable variable substitutions, as we
<td width="25%">None</td>
</tr>
<tr>
<td width="25%"><code>amp-pixel</code><br><a href="https://github.com/ampproject/amphtml/blob/main/builtins/amp-pixel.md#substitutions">Detailed documentation</a></td>
<td width="25%"><code>amp-pixel</code><br><a href="https://github.com/ampproject/amphtml/blob/main/builtins/amp-pixel/amp-pixel.md#substitutions">Detailed documentation</a></td>
<td width="25%">Requests must be HTTPS URLs (not a requirement specific to variable substitutions)</td>
<td width="25%">No</td>
<td width="25%">None</td>
Expand Down
6 changes: 3 additions & 3 deletions src/service/core-services.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,14 +28,14 @@ import {installGlobalNavigationHandlerForDoc} from './navigation';
import {installGlobalSubmitListenerForDoc} from '../document-submit';
import {installHiddenObserverForDoc} from './hidden-observer-impl';
import {installHistoryServiceForDoc} from './history-impl';
import {installImg} from '../../builtins/amp-img';
import {installImg} from '../../builtins/amp-img/amp-img';
import {installInaboxResourcesServiceForDoc} from '../inabox/inabox-resources';
import {installInputService} from '../input';
import {installLayout} from '../../builtins/amp-layout';
import {installLayout} from '../../builtins/amp-layout/amp-layout';
import {installLoadingIndicatorForDoc} from './loading-indicator';
import {installMutatorServiceForDoc} from './mutator-impl';
import {installOwnersServiceForDoc} from './owners-impl';
import {installPixel} from '../../builtins/amp-pixel';
import {installPixel} from '../../builtins/amp-pixel/amp-pixel';
import {installPlatformService} from './platform-impl';
import {installPreconnectService} from '../preconnect';
import {installResourcesServiceForDoc} from './resources-impl';
Expand Down
2 changes: 1 addition & 1 deletion test/integration/test-amp-pixel.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
* limitations under the License.
*/

import {AmpPixel} from '../../builtins/amp-pixel';
import {AmpPixel} from '../../builtins/amp-pixel/amp-pixel';
import {BrowserController, RequestBank} from '../../testing/test-helper';
import {createElementWithAttributes} from '../../src/dom';

Expand Down
2 changes: 1 addition & 1 deletion test/unit/test-amp-img-intrinsic.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ import {BrowserController} from '../../testing/test-helper';
import {applyStaticLayout} from '../../src/layout';
import {createElementWithAttributes} from '../../src/dom';
import {createIframePromise} from '../../testing/iframe';
import {installImg} from '../../builtins/amp-img';
import {installImg} from '../../builtins/amp-img/amp-img';
import {toArray} from '../../src/core/types/array';

describes.sandboxed('amp-img layout intrinsic', {}, () => {
Expand Down
2 changes: 1 addition & 1 deletion test/unit/test-amp-img-v1.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
* limitations under the License.
*/

import {AmpImg} from '../../builtins/amp-img';
import {AmpImg} from '../../builtins/amp-img/amp-img';
import {BaseElement} from '../../src/base-element';
import {Layout, LayoutPriority} from '../../src/layout';
import {createElementWithAttributes, dispatchCustomEvent} from '../../src/dom';
Expand Down
2 changes: 1 addition & 1 deletion test/unit/test-amp-img.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
* limitations under the License.
*/

import {AmpImg, installImg} from '../../builtins/amp-img';
import {AmpImg, installImg} from '../../builtins/amp-img/amp-img';
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's just move it to amp-img/index.js and then imports don't have to change?

Copy link
Member Author

Choose a reason for hiding this comment

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

I considered this. Every single amp component in the entire repo has the filename amp-component.js. If you feel strongly I'm ok with breaking that pattern here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm that's a very good point, although they live in 0.1 etc directory so it doesn't look as weird on import. I would be fine with breaking that pattern here since they're not extensions as much as they are part of the runtime, but I also don't feel strongly. Curious what others think. What's the motivation behind splitting the directories up here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Right now there are only 2 files per component (.md and .js) so lumping all in the same dir wasn't too bad.

I plan on introducing a third, in which case it starts becoming a bit disorganized

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm quite split between the two points. Could we either poll the group at stand-up or pull in a third opinion?

Copy link
Member Author

@samouri samouri May 6, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

poll taken, amp-img/amp-img wins. Also very ok with switching to index.js on a later date if the tides of opinion sway :)

import {BaseElement} from '../../src/base-element';
import {Layout, LayoutPriority} from '../../src/layout';
import {Services} from '../../src/services';
Expand Down