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

Jump to bottom

Add downloads samples #934

Open
wants to merge 25 commits into
base: main
Choose a base branch
from
Open

Add downloads samples #934

wants to merge 25 commits into from

Conversation

daidr
Copy link
Contributor

@daidr daidr commented May 31, 2023

No description provided.

@daidr daidr marked this pull request as ready for review June 5, 2023 11:17
@daidr
Copy link
Contributor Author

daidr commented Jun 5, 2023

The special thing is that eval was used before download_filename_controller to support writing js to modify the name of the downloaded file, but this cannot be used in MV3.

oliverdunk
oliverdunk requested changes Aug 25, 2023
View reviewed changes
Copy link
Member

@oliverdunk oliverdunk 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 few initial thoughts - thanks for your patience on this one!

api-samples/downloads/download_filename_controller/README.md Outdated Show resolved Hide resolved
api-samples/downloads/download_filename_controller/service-worker.js Outdated Show resolved Hide resolved
api-samples/downloads/download_manager/service-worker.js Outdated Show resolved Hide resolved
api-samples/downloads/download_manager/service-worker.js Show resolved Hide resolved
api-samples/downloads/download_manager/service-worker.js Outdated Show resolved Hide resolved
api-samples/downloads/download_manager/popup.js Outdated Show resolved Hide resolved
jpmedley
jpmedley suggested changes Aug 25, 2023
View reviewed changes
Copy link
Contributor

@jpmedley jpmedley left a comment

Choose a reason for hiding this comment

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

There isn't inherently anything wrong with what you wrote. The issue is that we need consistency for most of the changes I requested. It's no reasonable to expect a guest contributor to know that.

api-samples/downloads/download_filename_controller/README.md Outdated Show resolved Hide resolved
api-samples/downloads/download_filename_controller/README.md Outdated Show resolved Hide resolved
api-samples/downloads/download_filename_controller/README.md Outdated Show resolved Hide resolved
api-samples/downloads/download_filename_controller/manifest.json Outdated Show resolved Hide resolved
api-samples/downloads/download_links/README.md Outdated Show resolved Hide resolved
api-samples/downloads/download_manager/_locales/en/messages.json Show resolved Hide resolved
api-samples/downloads/downloads_overwrite/README.md Outdated Show resolved Hide resolved
api-samples/downloads/downloads_overwrite/README.md Outdated Show resolved Hide resolved
api-samples/downloads/downloads_overwrite/manifest.json Outdated Show resolved Hide resolved
api-samples/downloads/download_manager/service-worker.js Show resolved Hide resolved
@jpmedley
Copy link
Contributor

READMEs and Descriptions: LGTM

jpmedley
jpmedley suggested changes Aug 28, 2023
View reviewed changes
Copy link
Contributor

@jpmedley jpmedley left a comment

Choose a reason for hiding this comment

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

I stumbled on a typo when I was looking at where you replaced messages.json text with manifest.json text. I think I made the typo in one of my suggestions. Sorry about that.

@John-hivo
Copy link

Could you add the example for download open. I could not using it currently

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

@oliverdunk oliverdunk oliverdunk requested changes

@jpmedley jpmedley jpmedley approved these changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@daidr @jpmedley @John-hivo @oliverdunk