refactor SB* file uploader into top-level HOC #4809

benjiwheeler · 2019-05-10T19:10:56Z

Resolves

Big step towards resolving #4762
Resolves #3365

Proposed Changes

move sb-file-uploader to a top-level HOC that wraps gui
in situation where user has selected to upload project, but file content is not ready/available, dispatch project-state action RETURN_TO_SHOWING

Reason for Changes

we made design decision to use custom confirm modal, to have greater confidence in and control of the user experience when the user is at risk of overwriting their work. But this is not doable yet because clicking on the modal is interpreted as a click outside the open file menu, which closes the file menu. This is a problem because...
previously, the sb-file-uploader would be unmounted any time the file menu closed; its existence/mounting should not depend on a menu.
in testing, when i would clear the contents of the file to upload before it could be loaded into the vm, sb-file-uploader-hoc would correctly avoid loading the empty content; but it would be stuck in the LOADING_VM_FILE_UPLOAD state. Submitting RETURN_TO_SHOWING in this case makes sure we will stay in the right state in that case. (We shouldn't actually be missing the contents of the file to upload in practice, but just in case!)

Test Coverage

Added a test and moved over tests to new HOC, but ideally we would test even more.

benjiwheeler · 2019-05-15T00:05:17Z

src/reducers/project-state.js

+            };
+        case LoadingState.LOADING_VM_NEW_DEFAULT:
+            return {
+                type: START_ERROR


If loading fails (success === false), recover by going back to just showing project as-is. But if we were loading the default project, a failure in loading probably indicates a serious error.

benjiwheeler · 2019-05-15T00:05:52Z

test/unit/util/project-saver-hoc.test.jsx

    });

-    test('if canSave is alreatdy true and we show a project with an id, project will NOT be saved', () => {
+    test('if canSave is already true and we show a project with an id, project will NOT be saved', () => {


fixed a typo

benjiwheeler · 2019-05-15T00:07:26Z

src/lib/sb-file-uploader-hoc.jsx

@@ -0,0 +1,241 @@
+import bindAll from 'lodash.bindall';


I tried to keep as much as possible in this file the same as before. There are very few differences, aside from what was necessary to refactor it into a top-level HOC wrapping GUI.

I tried to keep as much as possible in this file the same as before. There are very few differences, aside from what was necessary to refactor it into a top-level HOC wrapping GUI.

Now there are more differences -- as part of making the component dynamic, I reordered functions and added comments.

benjiwheeler · 2019-05-15T00:08:15Z

Ready for review; just blocking until after this week's deploy.

benjiwheeler · 2019-05-21T15:53:34Z

@paulkaplan and I talked about this. Some takeaways:

Concerns:

Z is currently working to start loading project before gui fully renders. Adding a top level HOC, with an additionally rendered input component, can slow this down.
need to assess/measure/understand how much this increases the overall performance burden on gui.

Possible alternative approaches:

in containers/menu.jsx, mouseup events cause the menu to be closed. This is the original problem leading to this PR's refactor. What if instead, we just checked if the mouse target was a modal, and didn't close the menu then?

Next step:

pick this back up when we have more time to discuss. Possibly with @rschamp .

Labeling as blocked, per need for discussion.

src/reducers/project-state.js

rschamp · 2019-06-06T19:39:37Z

src/lib/sb-file-uploader-hoc.jsx

+                        onStartSelectingFileUpload={this.handleStartSelectingFileUpload}
+                        {...componentProps}
+                    />
+                    {fileInput}


Let's try to create this element dynamically in the handler, rather than mounting it here, to avoid issues with components unmounting.

benjiwheeler · 2019-06-06T20:28:01Z

Discussed with @rschamp and @paulkaplan. Current plan:

The menu click issue that prompted this should be fixed in and of itself
I should pull out my project state tests and state fix, and introduce those separately (done in reset title on transitioning to isShowingWithoutId #5291 )
The custom modal for project upload can then be introduced
Redo this PR as only a refactor, which doesn't change behavior at all
- change placement of the file picker to be a createelement that’s not even on the dom, per Ray's comment: "Let's try to create this element dynamically in the handler, rather than mounting it here, to avoid issues with components unmounting."

draft of refactor to make custom confirm modal work for upload; need to further edit sb-file-uploader Add support for custom confirm modal to sb-file-uploader Show custom confirm modal when confirming project upload got custom upload refactor of file upload working, and tested failure case cleaned up debug code, make project file upload work simplified logic around canceling file upload removed old sb-file-uploader.jsx; fixed linter errors reverted two components to use old strings, not shared (can do that refactor another time) removed unnecessary confirm container removed stray comment moved, updated sb file uploader tests, related project state tests removed custom modal, to reduce scope of changes sb file uploader test uses intl

benjiwheeler · 2020-01-30T22:22:27Z

This PR is ready for review, but tests aren't passing; it's difficult to diagnose because the same tests pass locally.

Update: tests are fixed.

benjiwheeler · 2020-02-03T16:54:27Z

OK @rschamp, take a look at the dynamic way sb-file-uploader-hoc is now creating the element.

rschamp

Looks good to me. I found a couple of comments that seemed superfluous.

Also on comments, I personally find the "step #" style comments distracting because they imply a "correct" route for reading the code, so if you encounter a step number out of context you have to read it from beginning to end to understand what it's talking about. To convey the same type of information, I think I'd rather see a flow diagram showing the relationship between the functions at the top. Just my 2¢, not asking you to change it.

src/lib/sb-file-uploader-hoc.jsx

benjiwheeler added the pr - changes requested label May 10, 2019

benjiwheeler self-assigned this May 10, 2019

benjiwheeler added this to the May 2019 milestone May 10, 2019

benjiwheeler force-pushed the refactor-file-uploader branch 2 times, most recently from 1b34449 to 4820132 Compare May 10, 2019 20:01

benjiwheeler changed the title ~~Custom SB* file upload confirm modal; refactor file uploader into top-level HOC~~ refactor SB* file uploader into top-level HOC May 14, 2019

benjiwheeler force-pushed the refactor-file-uploader branch from 2ce692c to 28a120a Compare May 14, 2019 19:51

benjiwheeler requested a review from paulkaplan May 14, 2019 19:52

benjiwheeler commented May 15, 2019

View reviewed changes

benjiwheeler added blocked pr - needs review and removed pr - changes requested labels May 15, 2019

benjiwheeler assigned paulkaplan and unassigned benjiwheeler May 20, 2019

benjiwheeler added needs discussion and removed pr - needs review labels May 21, 2019

benjiwheeler assigned rschamp and benjiwheeler and unassigned paulkaplan May 21, 2019

benjiwheeler modified the milestones: May 2019, Backlog May 21, 2019

rschamp suggested changes Jun 6, 2019

View reviewed changes

benjiwheeler removed the needs discussion label Jun 6, 2019

benjiwheeler unassigned rschamp Jun 6, 2019

benjiwheeler mentioned this pull request Jan 6, 2020

Menus should not close themselves on mouseup, if mouse is in a modal #5363

Closed

benjiwheeler modified the milestones: Backlog, January 2020 Jan 16, 2020

benjiwheeler added 2 commits January 28, 2020 14:57

file input is dynamically added to DOM, not rendered

efed4d3

benjiwheeler force-pushed the refactor-file-uploader branch from dd7b81b to efed4d3 Compare January 30, 2020 00:39

benjiwheeler added the pr - changes requested label Jan 30, 2020

reverted project state changes in f2291d9

48245be

update menu-bar test per refactor

59f4289

benjiwheeler added pr - needs review and removed pr - changes requested labels Feb 3, 2020

benjiwheeler assigned rschamp and unassigned benjiwheeler Feb 3, 2020

benjiwheeler modified the milestones: January 2020, February 2020 Feb 4, 2020

rschamp previously approved these changes Feb 4, 2020

View reviewed changes

src/lib/sb-file-uploader-hoc.jsx Outdated Show resolved Hide resolved

src/lib/sb-file-uploader-hoc.jsx Outdated Show resolved Hide resolved

rschamp added pr - good to merge and removed pr - needs review labels Feb 4, 2020

rschamp assigned benjiwheeler and unassigned rschamp Feb 4, 2020

benjiwheeler added the pr - changes requested label Feb 4, 2020

removed stale comments

d19cf91

benjiwheeler dismissed rschamp’s stale review via d19cf91 February 4, 2020 22:31

benjiwheeler removed the pr - changes requested label Feb 4, 2020

benjiwheeler merged commit 5d9afb0 into scratchfoundation:develop Feb 4, 2020

benjiwheeler deleted the refactor-file-uploader branch February 4, 2020 22:32

This was referenced Feb 6, 2020

Revert "refactor SB* file uploader into top-level HOC" #5436

Closed

Revert "refactor SB* file uploader into top-level HOC" #5437

Merged

benjiwheeler mentioned this pull request Feb 24, 2020

Revert "Revert "refactor SB* file uploader into top-level HOC"" #5450

Merged

refactor SB* file uploader into top-level HOC #4809

refactor SB* file uploader into top-level HOC #4809

Uh oh!

Conversation

benjiwheeler commented May 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Resolves

Proposed Changes

Reason for Changes

Test Coverage

Uh oh!

benjiwheeler May 15, 2019

Choose a reason for hiding this comment

Uh oh!

benjiwheeler May 15, 2019

Choose a reason for hiding this comment

Uh oh!

benjiwheeler May 15, 2019

Choose a reason for hiding this comment

Uh oh!

benjiwheeler Jan 30, 2020

Choose a reason for hiding this comment

Uh oh!

benjiwheeler commented May 15, 2019

Uh oh!

benjiwheeler commented May 21, 2019

Uh oh!

Uh oh!

rschamp Jun 6, 2019

Choose a reason for hiding this comment

Uh oh!

benjiwheeler commented Jun 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

benjiwheeler commented Jan 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

benjiwheeler commented Feb 3, 2020

Uh oh!

rschamp left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

benjiwheeler commented May 10, 2019 •

edited

Loading

benjiwheeler commented Jun 6, 2019 •

edited

Loading

benjiwheeler commented Jan 30, 2020 •

edited

Loading