[Student Libraries] Exporter UI #31303

epeach · 2019-10-16T18:42:09Z

Description

Part of the Student Libraries work.
When the user goes to 'Share' -> 'Share as Library' this is what they see.

Note:
Not i18n until wording confirmed.

Note: Can't submit with an empty description or with no checkboxes checked. Can't check a checkbox that doesn't have a comment.

Links

spec

Testing story

Manually tested.

Reviewer Checklist:

Tests provide adequate coverage
Code is well-commented
New features are translatable or updates will not break translations
Relevant documentation has been added or updated
User impact is well-understood and desirable
Pull Request is labeled appropriately
Follow-up work items (including potential tech debt) are tracked and linked

jmkulwik · 2019-10-16T19:56:59Z

apps/src/code-studio/components/Libraries/LibraryCreationDialog.jsx

+        <Body>
+          <PadAndCenter>
+            <div style={styles.libraryBoundary}>
+              <Heading1>Functions as a Library</Heading1>


Maybe "Export functions as a library?" (also posted in #student-created-libraries where more of the UI review is happening)

jmkulwik · 2019-10-16T20:04:11Z

apps/src/code-studio/components/Libraries/LibraryCreationDialog.jsx

+            style={styles.textarea}
+            placeholder="Write a description of your library"
+          />
+          {this.state.selectedFunctionList.map(selectedFunction => {


When 'publish' is clicked, only the functions whose boxes are checked should be in "selectedFunctionsList."

I didn't add this originally, but we'll probably need two lists of functions, the original functions parsed from the code, and the list of functions that have been checked.

Whoops - forgot to only send the selected functions through. 🤦‍♀️ Fixing.

I don't understand your second comment though. Can you elaborate?

Haha it was meant to clarify the first comment, but sounds like it wasn't necessary (or helpful lol).

For the createLibrary - I swapped out the full list of functions to be just the selected ones. Can you confirm that's the correct usage? (Latest commit)

👍 Thank you!

jmkulwik · 2019-10-16T20:08:52Z

apps/src/code-studio/components/Libraries/LibraryCreationDialog.jsx

 import {connect} from 'react-redux';
 import {hideLibraryCreationDialog} from '../shareDialogRedux';
 import libraryParser from './libraryParser';
 import LibraryClientApi from './LibraryClientApi';
 import i18n from '@cdo/locale';
+import PadAndCenter from '../../../templates/teacherDashboard/PadAndCenter';
+import {Heading1, Heading2} from '../../../lib/ui/Headings';


Might be good to use @cdo format for these two as well given the three levels of ../.

jmkulwik · 2019-10-16T20:27:43Z

Make sure you add tests!
Some already exist under /code-dot-org/apps/test/unit/code-studio/components/libraries/

jmkulwik · 2019-10-17T19:33:17Z

apps/src/code-studio/components/Libraries/LibraryCreationDialog.jsx

            return (
              <div key={keyIndex++} style={styles.functionItem}>
                <input
                  type="checkbox"
                  style={styles.largerCheckbox}
                  disabled={comment.length === 0}
                  onClick={this.validateInput}
+                  value={keyIndex}


What does this do? (and why is the default -1?)

Good question! This was a workaround for handling functions that have the same name/comments. Since we can't rely on unique values to connected the selected checkbox to the list of functions, I've shifted to using indices. Because javascript .map goes over the array items in order, we can assign each item created an index so that we can then track that index back to the function it is meant to represent.

It is initialized at -1 because it increments on each item created, so it has to start at -1 so that it starts zero indexed. I don't love this solution, but I couldn't really figure out another way to do while still using a map. I'm open to suggestions!

Don't love that it starts at -1?
You should be able to increment the counter at the end of .map instead of the beginning of .map. Then you can start it at 0.

jmkulwik · 2019-10-17T19:35:03Z

apps/src/code-studio/components/Libraries/LibraryCreationDialog.jsx

    let libraryJson = libraryParser.createLibraryJson(
      this.state.librarySource,
-      this.state.selectedFunctionList,
+      selectedFunctionList,
      this.state.libraryName


Were you planning to add libraryDescription here?

Yes, still in progress.

epeach · 2019-10-17T20:45:28Z

@jmkulwik PTAL - while implementing tests I realized that there are something in the state that are probably best suited to not be part of state. I am tracking an work item now to fix that at a later point and add some more tests, but I don't think it's worth blocking this PR on it.

jmkulwik · 2019-10-17T22:23:23Z

apps/src/code-studio/components/Libraries/LibraryCreationDialog.jsx

+                {comment.length === 0 && (
+                  <p style={styles.alert}>
+                    This function cannot be exported until you add a comment to
+                    it.


I was planning to hold off on i18n until we had a better idea if these were the final phrasings. Do you think we're there?

jmkulwik · 2019-10-17T22:28:18Z

apps/src/code-studio/components/Libraries/LibraryCreationDialog.jsx

+      if (element.type === 'checkbox' && element.checked) {
+        selectedFunctionList.push(this.state.sourceFunctionList[element.value]);
+      }
+      if (element.type === 'textarea') {


NIT: (I don't feel too strongly about this) This seems like it would be more maintainable as a check by class name rather than type. i.e. it's not too unlikely that we'd add another textarea somewhere.

epeach · 2019-10-17T23:16:04Z

@jmkulwik PTAL - thanks!

jmkulwik · 2019-10-21T18:12:34Z

apps/test/unit/code-studio/components/libraries/LibraryCreationDialogTest.js

+    expect(wrapper.find(CHECKBOX_SELECTOR).last()).to.be.disabled();
+  });
+
+  it('displays loading while in the loading state', () => {


jmkulwik · 2019-10-21T18:13:34Z

apps/test/unit/code-studio/components/libraries/libraryParserTest.js

@@ -130,10 +175,16 @@ describe('Library parser', () => {

      let expectedFunctions = `${firstFunctionName}: ${firstFunctionName},${secondFunctionName}: ${secondFunctionName}`;


Maybe add a test with a non-empty description

Added to: https://codedotorg.atlassian.net/browse/STAR-765

jmkulwik · 2019-10-21T18:14:52Z

apps/src/code-studio/components/Libraries/LibraryCreationDialog.jsx

  displayFunctions = () => {
    if (!this.state.loadingFinished) {
-      return <div>Loading...</div>;
+      return <div id="loading">Loading...</div>;


If you get the chance, it would be nice to make this a spinner instead of Loading...

https://codedotorg.atlassian.net/browse/STAR-775

Erin Peach added 6 commits October 15, 2019 10:44

Update Dialog Styles

2e12bb7

Add form element

4667d00

Display function comments and fix scrolling

f869c66

Add alert for no comments. Update comment and dialog styling

68f1e98

Style fixes

2cb88dd

Add form validation

03d06bb

epeach added the star-labs label Oct 16, 2019

jmkulwik reviewed Oct 16, 2019

View reviewed changes

Erin Peach added 2 commits October 16, 2019 13:39

PR style fixes

ca2836f

Only submit selected libraries

c6be322

jmkulwik reviewed Oct 17, 2019

View reviewed changes

epeach changed the title ~~[Student Libraries] Exporter UI~~ WIP [Student Libraries] Exporter UI Oct 17, 2019

Erin Peach added 2 commits October 17, 2019 13:15

Add basic ui test coverage

6d215d7

Use description

5f8d3bc

epeach changed the title ~~WIP [Student Libraries] Exporter UI~~ [Student Libraries] Exporter UI Oct 17, 2019

jmkulwik reviewed Oct 17, 2019

View reviewed changes

Erin Peach added 2 commits October 17, 2019 16:04

PR Feedback clean-up

8077f2b

Update failing library parser tests with description

7347b3f

Fix tests

34b2c15

jmkulwik reviewed Oct 21, 2019

View reviewed changes

jmkulwik approved these changes Oct 21, 2019

View reviewed changes

epeach merged commit 091a9e1 into staging Oct 21, 2019

epeach deleted the exporter-ui branch October 21, 2019 18:20

This was referenced Oct 21, 2019

Revert "[Student Libraries] Exporter UI" #31378

Merged

Clean up library dialog #31416

Merged

Exporter ui spinner #31419

Merged

Adding spinner and test #31430

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Student Libraries] Exporter UI #31303

[Student Libraries] Exporter UI #31303

epeach commented Oct 16, 2019

jmkulwik Oct 16, 2019

epeach Oct 16, 2019

jmkulwik Oct 16, 2019

jmkulwik Oct 16, 2019

epeach Oct 16, 2019

jmkulwik Oct 16, 2019

epeach Oct 16, 2019

jmkulwik Oct 17, 2019

jmkulwik Oct 16, 2019

epeach Oct 16, 2019

jmkulwik commented Oct 16, 2019 •

edited

jmkulwik Oct 17, 2019

epeach Oct 17, 2019

epeach Oct 17, 2019

jmkulwik Oct 17, 2019

jmkulwik Oct 17, 2019

epeach Oct 17, 2019

epeach commented Oct 17, 2019

jmkulwik Oct 17, 2019

epeach Oct 17, 2019

jmkulwik Oct 17, 2019

epeach commented Oct 17, 2019

jmkulwik Oct 21, 2019

jmkulwik Oct 21, 2019

epeach Oct 21, 2019

jmkulwik Oct 21, 2019

epeach Oct 21, 2019

		@@ -130,10 +175,16 @@ describe('Library parser', () => {

		let expectedFunctions = `${firstFunctionName}: ${firstFunctionName},${secondFunctionName}: ${secondFunctionName}`;

[Student Libraries] Exporter UI #31303

[Student Libraries] Exporter UI #31303

Conversation

epeach commented Oct 16, 2019

Description

Links

Testing story

Reviewer Checklist:

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

jmkulwik commented Oct 16, 2019 • edited

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

epeach commented Oct 17, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

epeach commented Oct 17, 2019

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

jmkulwik commented Oct 16, 2019 •

edited