Skip to content
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

[WIP] Early implementation of serializable_models and user settings. #31

Closed
wants to merge 10 commits into from

Conversation

armcburney
Copy link
Member

@armcburney armcburney commented Oct 28, 2018

Overview

This pull request refactors large chunks of the state code pertaining to how we handle serialization/deserialization in the editor. It is currently a work in progress.

Serializable

Main features are the addition of a Serializable interface which frontend models that need to be serialized must implement. Anything we need to persist in local storage in the future should implement this interface.

/**
 * Serializable is an interface for frontend serializable_models to be stored
 * with localstorage.
 */
export interface Serializable<T> {
    // Takes the object and serialize it to a JSON string representation.
    serialize(): string;

    // Takes the a JSON string representation and updates the properties of the
    // object instance.
    deserialize(serialized: string);

     // Returns a representation of the object as a TypeScript type T.
    asBakedType(): T;

    // Sets the state of the object with a partial implementation of the
    // TypeScript type T.
    setState(newState: Partial<T>);

     // Cleans the object's state by clearing all the properties of the object.
    clearState();
}

Settings

The settings class will contain the user's configurable settings for the editor. It implements the Serializable interface, as we need to persist the user's settings in local storage. For more context, please see issue #29.

Todos

  • Implement the frontend for the Settings object
  • Implement the storage code for the Settings object (may entail refactor)
  • Document the State object
  • Document the Settings object
  • Write unit tests for State and Settings

};

export class State implements Serializable<BakedState> {
public generator?: calder.Generator;
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm going to rethink this. Right now this is exposing too much, making the class a big code smell, and violating a few design principles like the dependency inversion principle and liskov's substitution principle.

I'll probably make these private, and have any code which needs these properties access it through the asBakedType() method.

* type.
*/
asBakedType(): BakedState {
return {
Copy link
Member

Choose a reason for hiding this comment

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

Out of curiosity, does return this work here? It might mess up the { ...state.asBakedType() } later if it also tries to copy the methods and not just the properties, I'm not sure if the spread operator does that or not

Copy link
Member Author

Choose a reason for hiding this comment

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

Ahh, I'm not sure either. I'll look into it 👍

@davepagurek
Copy link
Member

The state and serializable refactor looks good so far!

fieldset input {
width: auto;
height: auto;
float: left;
Copy link
Member

Choose a reason for hiding this comment

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

do we need the float if it's in a group, which is display:flex? I feel like it might work without it

@armcburney
Copy link
Member Author

Closing in favour of #54.

@armcburney armcburney closed this Mar 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants