Skip to content

upload in current project id, rather than creating new project id #4700

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
Merged
merged 1 commit into from
Apr 17, 2019
Merged

upload in current project id, rather than creating new project id #4700

merged 1 commit into from
Apr 17, 2019

Conversation

@benjiwheeler
Copy link
Contributor

@benjiwheeler benjiwheeler commented Mar 30, 2019 •
edited
Loading

Resolves

Proposed Changes

  • for logged in users, uploading a project gives it the same project id as the current project, rather than creating a new project id.
  • confirm logic for upload is now: show confirm dialog if user owns current project, or project has been changed since it was opened.
  • removed related code from project-state.js that will no longer be used
  • sets the project filename from the upload file name (did not require code changes)
  • confirms after showing the upload dialog (did not require code changes)
  • shuts file menu if the confirm is cancelled
  • changes “Creating project” to “Loading project” for the blue screen

Still needs work

  • we plan to replace the browser confirm() with a custom modal

Note

  1. The new confirm logic will not confirm if you do the following:

    1. be logged out
    2. upload a project (we'll call it "Project A")
    3. immediately upload another project ("Project B")
      The reason is that you haven't made any alterations to Project A; it's as if you just clicked see inside on a project, and then uploaded on top of that.

  2. Replacing the project keeps the extensions open that the previous project was using. This issue is filed in Extensions should unload if you change projects scratch-vm#1731 and we do not plan on fixing it as part of this change.

Test Coverage

Revised test/unit/reducers/project-state-reducer.test.js to reflect changes

Browser Coverage

Check the OS/browser combinations tested (At least 2)

Mac

  • Chrome
  • Firefox
  • Safari

Windows

  • Chrome
  • Firefox
  • Edge

Chromebook

  • Chrome

iPad

  • Safari

Android Tablet

  • Chrome

@benjiwheeler benjiwheeler mentioned this pull request Mar 30, 2019
Closed
@benjiwheeler benjiwheeler self-assigned this Mar 30, 2019
@benjiwheeler benjiwheeler added this to the April 2019 milestone Mar 30, 2019
@benjiwheeler benjiwheeler force-pushed the upload-in-place branch 2 times, most recently from 84f09bf to a4e3683 Compare April 5, 2019 21:43
@benjiwheeler
Copy link
Contributor Author

benjiwheeler commented Apr 5, 2019

Current status of this PR:

  • clicking ok doesn't work
  • confirm modal design needs design approval, probably needs work
  • need to test all the scenarios:
    • viewing someone else's code
      • with changes
      • without changes
      • logged out
      • logged in
    • viewing your code
      • with changes
      • without changes
    • in standalone mode
      • with changes
      • without changes

@benjiwheeler
Copy link
Contributor Author

benjiwheeler commented Apr 6, 2019

Current draft looks like:

image

@benjiwheeler
Copy link
Contributor Author

benjiwheeler commented Apr 16, 2019

We are going to use javascript native confirm for now, rather than the new custom confirm.

@benjiwheeler benjiwheeler requested a review from paulkaplan April 17, 2019 03:36
benjiwheeler
benjiwheeler commented Apr 17, 2019
View reviewed changes
src/components/menu-bar/menu-bar.jsx
}
id="gui.menuBar.uploadFromComputer"
/>
{this.props.intl.formatMessage(sharedMessages.loadFromComputerTitle)}
Copy link
Contributor Author

@benjiwheeler benjiwheeler Apr 17, 2019

Choose a reason for hiding this comment

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

This message seems to me to make more sense living in a shared file than cluttering the render content of menu-bar

benjiwheeler
benjiwheeler commented Apr 17, 2019
View reviewed changes
src/components/menu-bar/menu-bar.jsx
sessionExists: state.session && typeof state.session.session !== 'undefined',
username: user ? user.username : null,
userOwnsProject: ownProps.authorUsername && user &&
(ownProps.authorUsername === user.username),
Copy link
Contributor Author

@benjiwheeler benjiwheeler Apr 17, 2019

Choose a reason for hiding this comment

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

I believe we discussed creating this prop -- it makes for cleaner separation of props logic and component display logic.

benjiwheeler
benjiwheeler commented Apr 17, 2019
View reviewed changes
src/containers/sb-file-uploader.jsx
// If user owns the project, or user has changed the project,
// we must confirm with the user that they really intend to replace it.
// (If they don't own the project and haven't changed it, no need to confirm.)
let uploadAllowed = true;
Copy link
Contributor Author

@benjiwheeler benjiwheeler Apr 17, 2019

Choose a reason for hiding this comment

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

I tried to go for legibility here. It's not the most concise code, but I hope this will minimize the chance that the logic will be misunderstood.

benjiwheeler
benjiwheeler commented Apr 17, 2019
View reviewed changes
src/reducers/project-state.js
const DONE_REMIXING = 'scratch-gui/project-state/DONE_REMIXING';
const DONE_UPDATING = 'scratch-gui/project-state/DONE_UPDATING';
const DONE_UPDATING_BEFORE_COPY = 'scratch-gui/project-state/DONE_UPDATING_BEFORE_COPY';
const DONE_UPDATING_BEFORE_FILE_UPLOAD = 'scratch-gui/project-state/DONE_UPDATING_BEFORE_FILE_UPLOAD';
Copy link
Contributor Author

@benjiwheeler benjiwheeler Apr 17, 2019

Choose a reason for hiding this comment

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

All of the changes to project-state are related to removing the "update (AKA save) project before uploading file" step, since we're no longer keeping around any changes to the current project.

benjiwheeler
benjiwheeler commented Apr 17, 2019
View reviewed changes
test/integration/menu-bar.test.js
'contains(@class, "menu-bar_hoverable")][span[text()="File"]]'
);
await findByXpath('//*[li[span[text()="Load from your computer"]] and not(@data-tip="tooltip")]');
await findByXpath('//*[li[text()="Load from your computer"] and not(@data-tip="tooltip")]');
Copy link
Contributor Author

@benjiwheeler benjiwheeler Apr 17, 2019

Choose a reason for hiding this comment

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

Since the message is in a shared file now, there's one less span

paulkaplan
paulkaplan approved these changes Apr 17, 2019
View reviewed changes
Copy link
Contributor

@paulkaplan paulkaplan left a comment

Choose a reason for hiding this comment

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

Code changes look good to me.

@benjiwheeler benjiwheeler merged commit f8243ea into scratchfoundation:develop Apr 17, 2019
@benjiwheeler benjiwheeler deleted the upload-in-place branch April 17, 2019 18:55
This was referenced Apr 18, 2019
Merged
Open
This was referenced Jan 6, 2020
Closed
Closed
This was referenced Jan 16, 2020
Open
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@paulkaplan paulkaplan paulkaplan approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@paulkaplan paulkaplan

Labels

pr - good to merge

Projects

None yet

Milestone

April 2019

Development

Successfully merging this pull request may close these issues.

Uploading a project creates a new project, rather than replacing the current project’s data

2 participants

@benjiwheeler @paulkaplan