Skip to content
This repository was archived by the owner on Mar 14, 2024. It is now read-only.

Add extension sample list page#7029

Merged
matthiasrohmer merged 40 commits into
GoogleChrome:mainfrom
daidr:daidr/samples-list
Aug 17, 2023
Merged

Add extension sample list page#7029
matthiasrohmer merged 40 commits into
GoogleChrome:mainfrom
daidr:daidr/samples-list

Conversation

@daidr

@daidr daidr commented Aug 10, 2023

Copy link
Copy Markdown
Contributor

Changes proposed in this pull request:

  • Add a new page listing all extension samples.

Test step

  1. Run npm install
  2. Run npm run build-external. If the GITHUB_APP_ID and GITHUB_APP_KEY environment variables are not set, you can try copying external/build/testdata/extension-samples.json directly to external/build/data/extension-samples.json.
  3. Run npm run dev
  4. The new page can be found here: http://localhost:8080/docs/extensions/samples/

@chrome-devrel-infra

chrome-devrel-infra Bot commented Aug 10, 2023

Copy link
Copy Markdown

✅ Preview (app) for commit 073e642 available at https://pr-7029-app-dot-dcc-staging.uc.r.appspot.com/.

@oliverdunk
oliverdunk self-requested a review August 10, 2023 08:32
@oliverdunk oliverdunk self-assigned this Aug 10, 2023
@oliverdunk oliverdunk added the extensions Issues related to extensions documentation. label Aug 10, 2023

@oliverdunk oliverdunk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Wow, this is almost ready! Left a few comments. Could you also add some steps to the description on how to test (I needed to copy files from the testdata folder to data) and also a screenshot of what this looks like?

Comment thread external/build/extension-samples.js Outdated
Comment thread external/build/extension-samples.js
@@ -0,0 +1,172 @@
{% extends "layouts/base.njk" %}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It looks like the Fugu showcase has the .njk files in site/en/fugu-showcase/index.njk. Should we do similar?

@daidr daidr Aug 10, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I have placed it in the layout because there is no specific language content in the njk template (localization-related content has been moved to the i18n file). Here it functions similarly to site/en/docs/extensions/index.md, which contains the English title and description.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@matthiasrohmer, any thoughts on this?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I can follow the train of thought, but following the fugu-showcase example here would be good. So, putting the contents of this file directly in site/en/docs/extensions/samples/index.md(and renaming it to njk`) - it's unlikely we'll have localized versions of this page (which is when the extraction would make sense) and it's more intuitive to do changes for new starters as they would find the code they expect inside the page rather than having to further search for the used template.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done. 7a20eba

Comment thread site/_includes/partials/sample-item.njk Outdated
Comment thread site/_js/extension-samples.js
Comment thread site/_js/extension-samples.js Outdated
Comment thread site/_includes/partials/sample-item.njk Outdated
Comment thread site/_includes/layouts/extension-samples.njk Outdated
Comment thread site/_includes/layouts/extension-samples.njk
Comment thread site/_scss/blocks/_sample-item.scss Outdated
daidr and others added 2 commits August 10, 2023 17:29
Co-authored-by: Oliver Dunk <oliver@oliverdunk.com>
@jpmedley

Copy link
Copy Markdown
Contributor

Guys,

This looks great. I can't wait to see it up.

Where are the sample descriptions drawn from (specifically)? They're inconsistent and sometimes missing. If we can't fix that before this is deployed. We need to do it asap.

@AmySteam

Copy link
Copy Markdown
Collaborator

@jpmedley It comes from the manifest "description" key

@oliverdunk

Copy link
Copy Markdown
Member

@sebastianbenz / @matthiasrohmer - I've done an initial review here and it's looking good to me! Would love your thoughts when you're able to :)

@matthiasrohmer matthiasrohmer left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for your work! Had a look, but didn't spot anything major.

As a heads-up: for this to be mergeable the test data needs to be manually put into the Cloud Storage bucket and then the checks need to kicked off. This ensures everything is in place for the build pipeline.

Comment thread package.json Outdated
Comment thread site/_includes/layouts/extension-samples.njk Outdated
Comment thread site/_includes/partials/sample-item.njk
Comment thread site/_includes/partials/sample-item.njk Outdated
Comment thread site/_js/extension-samples.js Outdated
@oliverdunk

Copy link
Copy Markdown
Member

@matthiasrohmer, could you take another look? Thanks!

@matthiasrohmer matthiasrohmer left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looks good to me once the lint errors are fixed. 🙂

Comment on lines +8 to +11
// The names of the samples matching the list will be hidden.
const IGNORED_SAMPLES = [
'sample.co2meter', // WebHID is not released yet.
];

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I am not particularly a fan of this, as this would require a change in this repo once the sample is ready to be released and constructs like this are often hard to debug ("Why does xyz not show up?"), but just take this as a note, no need to change 🙂

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

That thought ocurred to me, but I wasn't going to say anything since I'm not an engineer. Also, how else would we get this information?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Maybe it's possible to add a metadata in the sample (e.g. a field in the extension-samples.json file) that tells this script whether this sample should be ignored or not. This way you edit "sample is hidden" field at the same time when you edit the sample instead of a separate repo.

@@ -0,0 +1,172 @@
{% extends "layouts/base.njk" %}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I can follow the train of thought, but following the fugu-showcase example here would be good. So, putting the contents of this file directly in site/en/docs/extensions/samples/index.md(and renaming it to njk`) - it's unlikely we'll have localized versions of this page (which is when the extraction would make sense) and it's more intuitive to do changes for new starters as they would find the code they expect inside the page rather than having to further search for the used template.

@daidr

daidr commented Aug 17, 2023

Copy link
Copy Markdown
Contributor Author

Looks good to me once the lint errors are fixed. 🙂

I am a little confused because according to the logs, there seems to be a lint error in a dependency within the node_modules. However, this should not happen under normal circumstances, and I have not made any changes to the relevant code.

image

EDIT: I think it might be because I used some code from the gulp-tasks directory in an external script. The JavaScript files in the gulp-tasks directory are not being checked for syntax, but the ones in the external directory are. However, this error is still quite puzzling to me, and I'm not sure if I can solve it.

@matthiasrohmer
matthiasrohmer merged commit 81016fa into GoogleChrome:main Aug 17, 2023
@matthiasrohmer

Copy link
Copy Markdown
Collaborator

Thanks for all your work on this, @daidr! This is now live at https://developer.chrome.com/docs/extensions/samples/

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

Labels

extensions Issues related to extensions documentation.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants