Refactor to use local storage #8

mickmister · 2024-05-22T03:35:45Z

This PR adapts the application to use local storage to persist user-generated data.

A few directories have been added to the project:

actions - Functions that are called by components for user actions. Like addCommentToSection
hooks - React hooks that are called by components
client - Functions called by actions that get/set persistent information. this layer can be swapped for a separate backend later
store - Functions that interact with localStorage directly. These LocalStorageStore store functions are called by the LocalStorageClient client functions

I also want to move the components to a components directory as well, but I wanted to make the diff between the component files obvious so I kept the same locations for the component files in this PR.

https://deploy-preview-8--section-view.netlify.app

netlify · 2024-05-22T03:36:23Z

✅ Deploy Preview for section-view ready!

Name	Link
🔨 Latest commit	`a869535`
🔍 Latest deploy log	https://app.netlify.com/sites/section-view/deploys/664d785811cf6f00089b5ad6
😎 Deploy Preview	https://deploy-preview-8--section-view.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

mickmister · 2024-05-22T04:05:15Z

src/actions/useActions.ts

+    // const projectId = globalStore.getCurrentProjectId()
+    const projectId = 'project-1';
+
+    const addCommentToEntity = async (message: string, entityPointer: EntityPointer) => {


Here entityPointer is essentially the thing that the comment is going to be attached to. At the moment, comments can technically be attached to the following:

project

section

file

So if this comment is going to be added to project-1, then entityPointer would have this value:

{ entityType: 'project', entityId: 'project-1' }

mickmister · 2024-05-22T04:05:49Z

src/types.ts

+export enum EntityType {
+    PROJECT = 'project',
+    SECTION = 'section',
+    FILE = 'file',
 }

+export type EntityPointer = {
+    entityType: EntityType;
+    entityId: string;
+}


Here we define the EntityPointer type, which is used to attach objects to other objects

mickmister · 2024-05-22T04:06:47Z

src/types.ts

+export type FileData = {
+    projectId: string;
+    id: string;
+    title: string;
+} & EntityPointer;
+
+export type CommentData = {
+    projectId: string;
+    id: string;
+    username: string;
+    message: string;
+} & EntityPointer;


Things that can be attached to other things will have this & EntityPointer thing in its type. I think this reads well. If you think it would read better, we can inline the entityType and entityId fields on this type instead

mickmister · 2024-05-22T04:07:50Z

src/utils.ts

+export const matchesEntityPointer = (entityPointer: EntityPointer, entityType: EntityType, entityId: string): boolean => {
+    return entityPointer.entityType === entityType && entityPointer.entityId === entityId;
+}


This function is only for making code that calls this shorter. Not sure how much value it brings

I'm more concerned with how readable something is, both in terms of brevity and in terms of how clear it is what the code is doing, without any comments. Essentially using variable names such that the code itself is almost like reading comments.

What do you think?

Yeah I'm in full agreement. So how do you feel about that in the context of this code we're commenting on?

mickmister · 2024-05-22T04:10:08Z

src/Files.tsx

+    const globalStore = useGlobalStore();
+
+    return (
+        <div className="files">
+            <span>+ Files</span>
+            {files.map((file) => {
+                const numComments = globalStore.getCommentsForFile(file.id).length;


The useGlobalStore hook helps get arbitrary things from the "already loaded" data. All data for the current project will be loaded at all times, so this data will always be available

mickmister · 2024-05-22T04:25:26Z

src/index.tsx

+    const projectId = 'project-1';
+    const sectionId = 'section-1';


These will eventually be taken from the URL parameters instead of being hardcoded here

Love it. Nice and extendable with clear intentions

mickmister · 2024-05-22T04:26:32Z

src/store/LocalStorageStore.ts

+const LOCAL_STORAGE_KEY_PROJECTS = 'projects';
+const LOCAL_STORAGE_KEY_SECTIONS = 'sections';
+const LOCAL_STORAGE_KEY_FILES = 'files';
+const LOCAL_STORAGE_KEY_COMMENTS = 'comments';
+
+export type StoreData = {
+    projects: ProjectData[];
+    sections: SectionData[];
+    files: FileData[];
+    comments: CommentData[];
+}
+
+export class LocalStorageStore {
+    private ls: LocalStorageDependency;
+
+    private currentData: StoreData;
+
+    constructor(ls: LocalStorageDependency) {
+        this.ls = ls;
+        this.currentData = this.migrateLocalStorageStore();


This file should only need to be edited if we add more types of data to the store

mickmister · 2024-05-22T04:28:05Z

src/store/LocalStorageStore.ts

+    clear = () => {
+        this.ls.clear();
+        this.currentData = this.migrateLocalStorageStore();
+    }
+
+    initializeWithSampleData = () => {
+        this.clear();
+        this.setAllProjects(initialProjects);
+        this.setAllSections(initialSections);
+        this.setAllFiles(initialFiles);
+        this.setAllComments(initialComments);
+    }


I'm thinking we can have two buttons on the UI:

"Clear data" - Calls store.clear() and reloads the page

"Reset with sample data" - Calls store.initializeWithSampleData() and reloads the page

The reloading of the page allows the application to re-initialize from local storage appropriately

Very interesting. I'd love to know more about your thought process for having those be the two buttons. Especially from a data flow perspective

They're just for debugging really. Those buttons would not be present in the production version of the application. The data flow is "update local storage then restart the program through reloading the page". That's the least obtrusive way to put that debug functionality in there I think. No special case code that's like "seed this data then re-read from local storage" anywhere in the code

Not sure I understand your question fully though

mickmister · 2024-05-22T04:29:20Z

src/store/MockLocalStorageDependency.ts

+export class MockLocalStorageDependency<T extends Record<string, object>> implements LocalStorageDependency {
+    public currentData: T;
+
+    constructor(data: T) {
+        this.currentData = data;
+    }
+
+    getItem(key: string): string | null {
+        if (this.currentData[key]) {
+            return JSON.stringify(this.currentData[key]);
+        }
+
+        return null;
+    }
+
+    setItem(key: string, value: string): void {
+        this.currentData[key as keyof T] = JSON.parse(value);
+    }


This file won't need to change unless we want to call more methods on localStorage. It essentially mocks localStorage itself, at least the subset of its methods being used by the application

mickmister · 2024-05-22T04:36:00Z

src/SectionPage.tsx

+import {SectionTitle} from './SectionTitle';
+import {useGlobalStore} from './hooks/useGlobalStore';
+
+type SectionPageProps = {
+    projectId: string;
+    sectionId: string;
+}
+
+const SectionPage: React.FC<SectionPageProps> = ({projectId, sectionId}) => {
+    const globalStore = useGlobalStore();
+    const section = globalStore.getSection(sectionId);
+    const files = globalStore.getFilesForSection(sectionId);
+
+    const sectionPointer: types.EntityPointer = {
+        entityId: sectionId,
+        entityType: types.EntityType.SECTION,
+    };
+
+    return (
+        <div className="root">
+            <SectionTitle sectionId={sectionId} />
+            <ChordProgression chordProgression={section.chordProgression} />


I made some whitespace changes in the PR. Please let me know what you think about:

4 spaces indent instead of 2 spaces - I personally feel strongly about this. It helps me scan the code much quicker and efficiently this way. The one argument someone may have against it is that it can make the code look too indented. My argument against that is that could be a sign that you need to address the number of levels of indentation happening on that line of code

Named imports have no spacings inside the braces - I feel less strongly about this, but still prefer it

I agree, I think four spaces is a great standard. Greatly enhances readability, which always goes a long way, even if that's not a measurable thing

mickmister · 2024-05-22T04:51:29Z

@aelishRollo I've found that when you add more comments to the page, elements towards the top of the page start vertically shrinking. This is present in the css code on the main branch

One comment

Five comments

mickmister · 2024-05-22T04:55:39Z

@aelishRollo I recommend checking out this branch refactor-to-local-storage locally, and browsing the codebase through your IDE. This way you can see the current state of the files in your own environment without having to process diffs in GitHub's UI. You can also access things like Go to Definition that way too

I recommend first just looking at the component files, and see how they connect to the files that they import

mickmister · 2024-05-22T05:12:48Z

src/SectionPage.tsx

+    const globalStore = useGlobalStore();
+    const section = globalStore.getSection(sectionId);
+    const files = globalStore.getFilesForSection(sectionId);
+
+    const sectionPointer: types.EntityPointer = {
+        entityId: sectionId,
+        entityType: types.EntityType.SECTION,
+    };
+
+    return (
+        <div className="root">
+            <SectionTitle sectionId={sectionId} />
+            <ChordProgression chordProgression={section.chordProgression} />
+            <Files files={files} />
+            <Comments entityPointer={sectionPointer} />
+            <CreateComment entityPointer={sectionPointer} />
+        </div>


A better design of how props would be passed in here may be like this:

const SectionPage: React.FC<SectionPageProps> = ({projectId, sectionId}) => { const globalStore = useGlobalStore(); const actions = useActions(); const section = globalStore.getSection(sectionId); const files = globalStore.getFilesForSection(sectionId); const comments = globalStore.getCommentsForSection(sectionId); const sectionPointer: types.EntityPointer = { entityId: sectionId, entityType: types.EntityType.SECTION, }; const updateTitle = (newTitle: string) => { actions.updateSection(...); }; const uploadFileToSection = (fileContent: File) => { actions.uploadFileToSection(sectionId, fileContent); }; const addCommentToSection = (message: string) => { actions.addCommentToSection(sectionId, message); }; return ( <div className="root"> <SectionTitle title={section.title} onEdit={updateTitle} /> <ChordProgression chordProgression={section.chordProgression} /> <Files files={files} uploadFile={uploadFileToSection} /> <Comments comments={comments} /> <CreateComment addComment={addCommentToSection} /> </div> ); };

Then the child components can be as "dumb" as possible https://devdojo.com/krissanawat101/minimal-explanation-of-dumb-and-smart-components-in-react

The frontend is currently quite non-elastic, and will definitely need to be refactored in a lot of ways to make it more robust for different viewports, as well as supporting the functionality we're looking to implement.

I think having a centralized way of managing all of these will go a long. Maybe we just make issues for each little issue like this, and then I can go through and clean them all up. What do you think is the best way to systematically tackle all of these?

Yeah I think itemizing things is the way to go. I'll spend some time with that soon

mickmister added 3 commits May 19, 2024 22:34

initial take on refactoring to store and client

1143c93

connect section title to store

860f64c

make the app use localStorage

827f22b

mickmister requested a review from aelishRollo May 22, 2024 03:35

remote IStore interface to simplify. fix workflow test command

687bd5e

mickmister commented May 22, 2024

View reviewed changes

mickmister added 2 commits May 22, 2024 00:41

use typed in username for comments

70e958d

add import for index.css

a869535

mickmister commented May 22, 2024

View reviewed changes

aelishRollo merged commit bc5af78 into main Jun 1, 2024
11 checks passed

aelishRollo deleted the refactor-to-local-storage branch June 1, 2024 19:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor to use local storage #8

Refactor to use local storage #8

mickmister commented May 22, 2024 •

edited

Loading

netlify bot commented May 22, 2024 •

edited

Loading

mickmister May 22, 2024

mickmister May 22, 2024

mickmister May 22, 2024

mickmister May 22, 2024

aelishRollo May 23, 2024

mickmister May 24, 2024

mickmister May 22, 2024

mickmister May 22, 2024

aelishRollo May 23, 2024 •

edited

Loading

mickmister May 22, 2024

mickmister May 22, 2024

aelishRollo May 23, 2024

mickmister May 24, 2024

mickmister May 24, 2024

mickmister May 22, 2024

mickmister May 22, 2024 •

edited

Loading

aelishRollo May 23, 2024

mickmister May 24, 2024

mickmister commented May 22, 2024 •

edited

Loading

mickmister commented May 22, 2024 •

edited

Loading

mickmister May 22, 2024 •

edited

Loading

aelishRollo May 23, 2024

mickmister May 24, 2024

		const projectId = 'project-1';
		const sectionId = 'section-1';

Refactor to use local storage #8

Refactor to use local storage #8

Conversation

mickmister commented May 22, 2024 • edited Loading

netlify bot commented May 22, 2024 • edited Loading

✅ Deploy Preview for section-view ready!

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

aelishRollo May 23, 2024 • edited Loading

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

mickmister May 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mickmister commented May 22, 2024 • edited Loading

mickmister commented May 22, 2024 • edited Loading

mickmister May 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mickmister commented May 22, 2024 •

edited

Loading

netlify bot commented May 22, 2024 •

edited

Loading

aelishRollo May 23, 2024 •

edited

Loading

mickmister May 22, 2024 •

edited

Loading

mickmister commented May 22, 2024 •

edited

Loading

mickmister commented May 22, 2024 •

edited

Loading

mickmister May 22, 2024 •

edited

Loading