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

Zip downloading: added modal for successfully downloading zips in game #10255

Closed
wants to merge 1 commit into from
Closed

Zip downloading: added modal for successfully downloading zips in game #10255

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 1 addition & 3 deletions pxtblocks/layout.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -226,9 +226,7 @@ export function flow(ws: Blockly.WorkspaceSvg, opts?: FlowOptions) {
}

export function screenshotEnabled(): boolean {
const disableForMacIos = pxt.appTarget.appTheme.disableFileAccessinMaciOs && (pxt.BrowserUtils.isMac() || pxt.BrowserUtils.isIOS());
const disableForAndriod = pxt.appTarget.appTheme.disableFileAccessinAndroid && pxt.BrowserUtils.isAndroid();
return !disableForMacIos && !pxt.BrowserUtils.isIE() && !disableForAndriod;
return pxt.BrowserUtils.hasFileAccess() && !pxt.BrowserUtils.isIE();
}

export function screenshotAsync(ws: Blockly.WorkspaceSvg, pixelDensity?: number, encodeBlocks?: boolean): Promise<string> {
Expand Down
11 changes: 11 additions & 0 deletions pxtlib/browserutils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,6 +173,17 @@ namespace pxt.BrowserUtils {
return window?.innerWidth > pxt.BREAKPOINT_TABLET;
}

export function isInGame(): boolean {
const inGame = /inGame=1/i.exec(window.location.href);
return !!inGame;
}

export function hasFileAccess(): boolean {
const disableForMacIos = pxt.appTarget.appTheme.disableFileAccessinMaciOs && (pxt.BrowserUtils.isMac() || pxt.BrowserUtils.isIOS());
const disableForAndroid = pxt.appTarget.appTheme.disableFileAccessinAndroid && pxt.BrowserUtils.isAndroid();
return !disableForMacIos && !disableForAndroid;

}
export function noSharedLocalStorage(): boolean {
try {
return /nosharedlocalstorage/i.test(window.location.href);
Expand Down
29 changes: 27 additions & 2 deletions webapp/src/scriptmanager.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -318,6 +318,17 @@ export class ScriptManagerDialog extends data.Component<ScriptManagerDialogProps
this.setState({ sortedAsc: !sortedAsc });
}

renderDownloadDialog(fileName: string) {
return <>
<div>
{lf("Your projects were zipped and downloaded successfully!")}
<br />
<br />
{lf("Look for the file {0} on your computer. It might have a number before the .zip if you've downloaded before.", fileName)}
Copy link
Contributor

Choose a reason for hiding this comment

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

At least when I've done this, I get a popup where I can choose the location and name for the file (both in the browser and when running dev Minecraft build on Windows pointing to your uploaded target). I'm wondering if we really need this additional string, or if we can just say it was downloaded successfully and leave it at that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that's what happens for me, too. I added the extra string because it just felt a bit bare to me with just one line, but I can remove the extra stuff.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I prefer concise language here. This needs to be translated to lot of languages and number system might be different. Just say "Your projects successfully downloaded as a zip"

</div>
</>;
}

handleDownloadAsync = async () => {
pxt.tickEvent("scriptmanager.downloadZip", undefined, { interactiveConsent: true });

Expand Down Expand Up @@ -424,11 +435,25 @@ export class ScriptManagerDialog extends data.Component<ScriptManagerDialogProps

const zipName = `makecode-${targetNickname}-project-download.zip`

pxt.BrowserUtils.browserDownloadDataUri(datauri, zipName);

this.setState({
Copy link
Contributor

Choose a reason for hiding this comment

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

Was the reordering of download: null state and browserDownloadDataUri intentional? My assumption would be that we should stay in a downloading state until all the steps have finished, but I think I may not be understanding what browserDownloadDataUri does...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, I should move this back, thanks for pointing that out. I think I was reordering stuff to better understand how everything interacted and moving it didn't change anything.

The browserDownloadDataUri function is doing the actual steps to initiate the download of the content to the user's computer. The way we do that differs per method and browser, but regardless, that function holds the download action. For the in-game, chromium case, we are basically putting the contents in an <a> tag and then clicking the link to initiate the download. The logic in handleDownloadAsync is basically only handling the zipping part.

download: null
});

pxt.BrowserUtils.browserDownloadDataUri(datauri, zipName);
if (pxt.BrowserUtils.isInGame()) {
const downloadJsx = this.renderDownloadDialog(zipName);

setTimeout(async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the timeout waiting for the download to finish? 2 seconds may not be enough time for someone with a whole bunch of projects and very slow internet (though not sure how likely that is).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it is. That's a good point. I don't really want to increase the time for it off the bat, though, because if it's too long and the notification shows up way after downloading then it feels even more disjoint.

This is not my favorite solution, but I included the timeout because I couldn't find an event that could tell us that the user stopped interacting with the filesystem/download unfortunately. No timeout was even worse because the modal would show up even before the filesystem modal would show up.

await core.confirmAsync({
header: lf("Projects Downloaded..."),
jsx: downloadJsx,
hasCloseIcon: true,
hideAgree: true,
className: 'zipdownloaddialog',
})
}, 2000);
}
}

handleDownloadProgressClose = () => {
Expand Down Expand Up @@ -529,7 +554,7 @@ export class ScriptManagerDialog extends data.Component<ScriptManagerDialogProps
}
headerActions.push(<sui.Button key="delete" icon="trash" className="icon red"
text={lf("Delete")} textClass="landscape only" title={lf("Delete Project")} onClick={this.handleDelete} />);
if (numSelected > 1) {
if (numSelected > 1 && pxt.BrowserUtils.hasFileAccess()) {
headerActions.push(<sui.Button key="download-zip" icon="download" className="icon"
text={lf("Download Zip")} textClass="landscape only" title={lf("Download Zip")} onClick={this.handleDownloadAsync} />);
}
Expand Down