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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

馃殌 [devdash] Render file list statically as placeholder #20600

Merged
merged 14 commits into from Jan 31, 2019

Conversation

alanorozco
Copy link
Member

@alanorozco alanorozco commented Jan 30, 2019

Prevents flash of unloaded content!

Also:

  • automatically finds required AMP extensions in document string.
  • fixes a validation error for amp-mustache.

Closes #19896.

Copy link
Contributor

@torch2424 torch2424 left a comment

Choose a reason for hiding this comment

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

Super awesome you did this! Thank you so much! 馃槃

* limitations under the License.
*/

const thru = a => a;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we have a more descriptive name for this? 馃槃

Copy link
Contributor

Choose a reason for hiding this comment

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

Something like returnArguments?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's pretty self-descriptive, no? It's only used in composition and it's not being exported.

* @return {string}
* @template T
*/
const many = (fragments, opt_renderer) =>
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we have a more descriptive name for this? 馃槃

Copy link
Contributor

Choose a reason for hiding this comment

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

Something like, joinHtmlFragments?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, this is only used in the context of HTML fragments. If context is needed the export can be used by html = require('./html-helpers'), html.many(...). I would rather keep a succinct name.

Copy link
Member Author

Choose a reason for hiding this comment

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

But yeah, might be too broad. Renamed to joinFragments.

const {SettingsModal, SettingsOpenButton} = require('./settings');

const fileListEndpointPrefix = '/dashboard/api/listing';

const examplesPathRegex = /^\/examples\//;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we move these regex into its own file, or constants object? 馃

Copy link
Member Author

Choose a reason for hiding this comment

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

It's a little cluttered at the moment but I plan to split this file semantically. That way constants will retain meaning without being exported or cluttering.

const ampStateKey = (...keys) => keys.join('.');

const selectModeStateId = 'documentMode';
const selectModeStateKey = 'selectModePrefix';
Copy link
Contributor

Choose a reason for hiding this comment

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

Same for these types of variables. Can they ben in a constants object? 馃槃

Copy link
Member Author

Choose a reason for hiding this comment

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

Ditto as above.

<script type="application/json">
{"src": "/dashboard/api/listing?path=${basepath}"}
${JSON.stringify(state)}
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice! 馃憤 馃槃

Copy link
Contributor

@torch2424 torch2424 left a comment

Choose a reason for hiding this comment

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

Discussed offline, agreed that this requires some general cleanup. And will be done in a later PR. 馃槃

@@ -0,0 +1,106 @@
/**
* Copyright 2018 The AMP HTML Authors. All Rights Reserved.
Copy link
Contributor

Choose a reason for hiding this comment

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

License Year 馃槃

Copy link
Member Author

Choose a reason for hiding this comment

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

Whoops :)

@alanorozco alanorozco merged commit 1e94b2e into ampproject:master Jan 31, 2019
@alanorozco alanorozco deleted the devdash-list branch January 31, 2019 01:19
nbeloglazov pushed a commit to nbeloglazov/amphtml that referenced this pull request Feb 12, 2019
)

Prevents flash of unloaded content!

Also:

- automatically finds required AMP extensions in document string.
- fixes a validation error for `amp-mustache`.

Closes ampproject#19896.
noranazmy pushed a commit to noranazmy/amphtml that referenced this pull request Mar 22, 2019
)

Prevents flash of unloaded content!

Also:

- automatically finds required AMP extensions in document string.
- fixes a validation error for `amp-mustache`.

Closes ampproject#19896.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AMP Dashboard / app-index: Render the initial file list as the placeholder
4 participants