New export ux #28795

cpirich · 2019-05-29T05:58:36Z

In preparation for student testing this week, we have updated the Export user experience (under the exportExpo experiment flag)
The Export button has been added to the project header (when the experiment is enabled) like this:

The Export dialog shows a series of pages to guide the student through the export process (screenshots below)
- NOTE: some of the pages are disabled through comments. The long-term plan is captured in this spec: https://docs.google.com/document/d/1o-C8GOi45FCzUKPRnsyrNS8k10jUYOlrP2zIBPeO1X4/edit#
Added a new sms_download entry point so we can send Twilio SMS messages for this scenario like this:

…vice endpoint

cpirich · 2019-05-29T05:59:04Z

@ryansloan

ryansloan

Tweaked some strings

apps/src/code-studio/components/ExportAllowedDialog.jsx

dashboard/app/controllers/sms_controller.rb

ryansloan

Strings LGTM

Hamms

This looks awesome! I have a couple nits, and two larger comments:

First, I am super concerned about the size and complexity of the ExportAllowedDialog component. Can it be broken up into multiple components? Perhaps reconceived as a generic "dialog with multiple pages" component, and then the implementation logic moved into individual Components representing each page in sequence?

Second, looks like there are quite a few places in this using hardcoded English strings. Can these all be pulled out into i18n strings?

Hamms · 2019-05-30T22:25:35Z

apps/src/code-studio/components/ExportAllowedDialog.jsx

+          <p style={styles.title}>{titleText}</p>
+        </div>
+        <div style={styles.section}>
+          <p style={styles.p} dangerouslySetInnerHTML={{__html: headerText}} />


prefer using the UnsafeRenderedMarkdown component when we want to format strings

I just played with it now and it isn't really working. I can't put it inside a <p> tag. When I put it inside a <div>, it loses the styles I applied. Plus it instantiates a markdown processor that I don't need. Seems like a good idea to have a reusable component, but it doesn't seem like the right fit for this scenario.

Hamms · 2019-05-30T22:28:05Z

apps/src/code-studio/components/ExportAllowedDialog.jsx

+    } = this.getActionButtonInfo();
+    const showShareWarning =
+      !this.props.canShareSocial &&
+      (this.props.appType === 'applab' || this.props.appType === 'gamelab');


is there a specific reason why weblab is included in the "sharing enabled" logic, but not here?

Good question. This copied the logic from ShareAllowedDialog - out of caution, I suggested to @ryansloan that we stick with the same logic for exporting. But I didn't try to revisit that logic.

That said, I should probably just remove the weblab references since we don't plan to support exporting weblab sites as mobile apps.

Hamms · 2019-05-30T22:29:05Z

apps/src/code-studio/components/ExportAllowedDialog.jsx

+
+export default connect(
+  state => ({
+    allowExportExpo: state.pageConstants.allowExportExpo || false,


isn't the || false here redundant?

It ensures that we satisfy a PropTypes requirement

Hamms · 2019-05-30T22:29:54Z

apps/src/code-studio/components/ExportDialog.jsx

+      return <ExportAllowedDialog {...otherProps} />;
+    }
+
+    return <ShareDisallowedDialog />; // TODO: re-use or create ExportDisallowedDialog?


I'd very much prefer to either create an ExportDisallowedDialog or rename ShareDisallowedDialog to something more generic

Ok, I figured out a way to eliminate the extra component altogether. Trying to mimic the layers of warnings in the share dialog has been a little challenging. There are so many that have been bolted on!

cpirich · 2019-05-31T03:04:13Z

This looks awesome! I have a couple nits, and two larger comments:

First, I am super concerned about the size and complexity of the ExportAllowedDialog component. Can it be broken up into multiple components? Perhaps reconceived as a generic "dialog with multiple pages" component, and then the implementation logic moved into individual Components representing each page in sequence?

I'm ok with doing that, though I'd prefer to do it after we lock in on a design (post student testing).

Second, looks like there are quite a few places in this using hardcoded English strings. Can these all be pulled out into i18n strings?

As this is only exposed by applab/gamelab, I'm following the pattern that we use throughout those apps where we do use hardcoded strings and do not use i18n

islemaster

Very cool! I'm not quite ready to approve. I know this is still behind an experiment, but I'd like to see at least an initial test for each bit of new business logic. Specifically:

ExportAllowedDialog's publishExpoExport and publishAndGenerateApk methods.
ExportDialog's decision about whether export is allowed.
ProjectHeader and ProjectBackedHeader's new includeExportInProjectHeader props.
The new /sms/send_download route.

islemaster · 2019-05-31T15:30:43Z

apps/src/code-studio/components/ExportAllowedDialog.jsx

+      study: 'finish-dialog-export',
+      study_group: 'v1',
+      event: 'project-export',
+      project_id: dashboard.project && dashboard.project.getCurrentId(),


I think you can import project directly without referring to the dashboard global, and it'll work just as well.

islemaster · 2019-05-31T15:39:39Z

apps/src/code-studio/components/ExportAllowedDialog.jsx

+      return;
+    }
+
+    await this.publishExpoExport();


Is it weird, from a student perspective, that generating the APK here could result in us showing this Expo error?

Failed to publish project to Expo. Please try again later.

There's a slight action-at-a-distance pattern here by depending on the state changes caused by publishExpoExport. I also suspect that this await is likely, but not guaranteed, to wait until the async setState calls in publishExpoExport complete.

I might call exportApp directly from publishAndGenerateApk, trading a little duplication for more control over what we do with the Expo results.

This is really good feedback. My solution is a little different from what you proposed, because I know that for the iOS case, I need to do publishExpoExport(), but not the rest of the steps in publishAndGenerateApk(), so I want to keep them separate.

I've changed publishExpoExport() to return an object (essentially the exportResult or a similarly shaped object for the "already exported" or failure cases). This way we don't rely on the async setState() as a method to pass the results in this chained async scenario.

That sounds great. 👍

islemaster · 2019-05-31T15:40:47Z

apps/src/code-studio/components/ExportAllowedDialog.jsx

+      case 'publish':
+        return this.renderPublishPage();
+      case 'generating':
+        return this.renderGeneratingPage();


Calling out a personal preference for subcomponents over helper render methods, but that's easy to fix later.

Understood, I'd be happy to revisit this later.

islemaster · 2019-05-31T15:48:25Z

pegasus/sites.v3/code.org/public/js/csf-congrats.js

@@ -30,7 +27,6 @@ $(document).ready(function()
 });

 var petition;
-$(document).ready(function()
-{
+$(document).ready(function() {


Did this file change at all, or just get Prettier run on it?

just prettier. somewhat on accident, but I decided to leave it since this now seems to match our preferred style.

islemaster · 2019-05-31T15:53:00Z

apps/src/code-studio/components/exportDialogRedux.js

+      return {
+        ...state,
+        ...initialState,
+        isOpen: true


This seems like overkill.

overkill meaning using redux to track whether the dialog is open? I agree that it is odd, but I started by following the share dialog and reduced most of the code away. I decided to keep this part because it allows us to use the dialog in a more "modeless" fashion (where you could close it while the export is underway, and open it back up later). I'm still in discussions with @ryansloan over the UI metaphor we'd use to enable that.

I specifically meant ...state, ...initialState, isOpen: true when there's only the one property here. I think I see the spirit of the thing though. 😁

islemaster

👍 Updates look great. Thanks for adding good tests!

cpirich · 2019-06-03T19:56:45Z

Merging since #28886 has been merged to staging, resolving these unrelated failures that I was seeing above...

cpirich added 4 commits May 20, 2019 12:43

WIP: barebones export dialog

6769cb3

add several pages

ec10fdc

Prep export dialog flow for student testing, enable send_download ser…

2ddffa6

…vice endpoint

merge staging

52c48c2

cpirich requested review from Hamms and islemaster May 29, 2019 05:58

ryansloan suggested changes May 29, 2019

View reviewed changes

apps/src/code-studio/components/ExportAllowedDialog.jsx Outdated Show resolved Hide resolved

apps/src/code-studio/components/ExportAllowedDialog.jsx Outdated Show resolved Hide resolved

ryansloan reviewed May 29, 2019

View reviewed changes

dashboard/app/controllers/sms_controller.rb Outdated Show resolved Hide resolved

string changes

aa968a1

ryansloan approved these changes May 29, 2019

View reviewed changes

update ProjectHeaderTest to mount with Provider

437136c

Hamms reviewed May 30, 2019

View reviewed changes

islemaster suggested changes May 31, 2019

View reviewed changes

cpirich added 7 commits May 31, 2019 15:38

PR feedback

5856ff5

add ProjectExport test cases to ProjectHeaderTest

bc0e937

add ProjectExport test cases with new ProjectBackedHeaderTest.js

bafcbf6

add ExportDialogTest.js

05a6a61

add test case for send_download_url_to_phone

65ce13f

remove allowExportExpo prop, require exportApp prop

0b8f86d

Merge remote-tracking branch 'origin/staging' into cpirich/new-export-ux

925aa91

islemaster approved these changes Jun 3, 2019

View reviewed changes

PL: Fix unrelated tests.

6b3c9aa

cpirich merged commit 08416cd into staging Jun 3, 2019

cpirich deleted the cpirich/new-export-ux branch June 3, 2019 19:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New export ux #28795

New export ux #28795

cpirich commented May 29, 2019 •

edited

cpirich commented May 29, 2019

ryansloan left a comment

ryansloan left a comment

Hamms left a comment

Hamms May 30, 2019

cpirich May 31, 2019

Hamms May 30, 2019

cpirich May 31, 2019

cpirich May 31, 2019

Hamms May 30, 2019

cpirich May 31, 2019

Hamms May 30, 2019

cpirich May 31, 2019

cpirich commented May 31, 2019

islemaster left a comment

islemaster May 31, 2019

islemaster May 31, 2019

cpirich May 31, 2019

islemaster May 31, 2019

islemaster May 31, 2019

cpirich May 31, 2019

islemaster May 31, 2019

cpirich May 31, 2019

islemaster May 31, 2019

cpirich May 31, 2019

islemaster May 31, 2019

islemaster left a comment

cpirich commented Jun 3, 2019

New export ux #28795

New export ux #28795

Conversation

cpirich commented May 29, 2019 • edited

cpirich commented May 29, 2019

ryansloan left a comment

Choose a reason for hiding this comment

ryansloan left a comment

Choose a reason for hiding this comment

Hamms left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpirich commented May 31, 2019

islemaster left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

islemaster left a comment

Choose a reason for hiding this comment

cpirich commented Jun 3, 2019

cpirich commented May 29, 2019 •

edited