-
Notifications
You must be signed in to change notification settings - Fork 7
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.
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
Adds project save/load functionality to :play #37
base: main
Are you sure you want to change the base?
Conversation
Adds a slot for account button in play-pen-header, that can be specified via play-pen.
## 💸 TL;DR Implements Project-level saving and loading in Play. ## Details This includes: * Adding the Save/Load buttons in the Project menu * Dialogs for both Save and Load flows * Logic that proxies the actual storage to something behind the `ProjectStorageClient` interface, which can be swapped out later * Storing the current project in `sessionStorage`, so that it persists across reload but not across tabs * Making sure "New" projects clear the current stored project * Making sure we can load/save projects several times * Using the Pen title as the project name (both when saving and loading)
@@ -60,8 +60,8 @@ | |||
"gzip": "3.5 KB" | |||
}, | |||
"dist/play-pen.js": { | |||
"none": "31900 KB", | |||
"gzip": "5150 KB" | |||
"none": "32100 KB", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a pretty big bump. does this PR really add 200 KB?
@@ -150,10 +150,12 @@ export class PlayNewPenButton extends LitElement { | |||
<div class="container"> | |||
<button | |||
class="new-pen" | |||
@click=${() => | |||
@click=${() => { | |||
this.dispatchEvent(Bubble<string>('new-project', '')) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would you mind updating the HTMLElementEventMap at the top of the file? same thing for other events in the patch.
@@ -0,0 +1,35 @@ | |||
/** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would you mind using types over interfaces where possible? interfaces are open and can be merged (eg, HTMLElementEventMap); types are closed and defined at creation.
* This can be injected into play-pen to provide a different implementation. | ||
*/ | ||
export interface ProjectStorageClient { | ||
CreateProject(name: string): Promise<PlayProject> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this a Protobuf type? if not, can we use camelCase?
/** readonly */ | ||
id?: string | undefined | ||
name: string | ||
/** readonly */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we mark these with the readonly keyword?
|
||
declare global { | ||
interface HTMLElementEventMap { | ||
'edit-src': CustomEvent<string> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
copypasta?
@click=${() => | ||
this.dispatchEvent( | ||
new CustomEvent('open-export-dialog', { | ||
bubbles: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we use utils/bubble?
<input | ||
type="button" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why can't we use a button?
|
||
@property() src: string = '' | ||
|
||
@state() private _loading = false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔕 please type all members.
@@ -0,0 +1,118 @@ | |||
// Implementation of ProjectStorageClient backed by IndexedDB, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gunsch @ObsidianSnoo, this adds another data source and removes some of the focus of play as a dumb fiddle. I think we should get together next week and chat about realistic goals for play to avoid remaking the organic mistakes of Studio. Ryan and I chatted earlier in the year about needing to do some refactors for internal vs external features, single file support, and other topics but haven't had much time to work on it.
💸 TL;DR
Replaces the "Export" button with a "Project" drop-down menu that includes "Save", "Load", and "Export".
"Save" and "Load" allow the user to develop on multiple projects at a time.
📜 Details
Some notes on implementation:
Storage and data
Projects
, which each haveProjectFiles[]
, to allow for building multi-file storage later. (though just runs with one file for now)UI
Project doc
Jira: add project menu
Jira: add project save/load functionality
🧪 Testing Steps / Validation
This was built during snoosweek with a lot of eyes on it from me, Andrei, and Emi! Trying to get it cleaned up and landed now.
Menu:
Save dialog:
Load dialog:
✅ Checks