Compare since previous compare PR #4

mickmister · 2024-04-15T22:26:12Z

https://section-view.netlify.app

…driven

mickmister

Nice work @aelishRollo! 🚀 🤫 This is amazing. I can't wait to be a user of this functionality

The improvements on the code are fantastic 👍

I have a few more comments regarding:

Creating types to use as props
Separating definitions of components to be outside of the App component's definition
Moving test data to its own file
Using React key prop for arrays of elements

src/App.tsx

mickmister · 2024-04-15T23:14:41Z

src/App.tsx

+
+  // Components
+
+  function Files() {


We have all of the components defined inside of this App component, as well as the creation of the test data happening inside of the component. This has a side effect of making it so React has no context of when things "change" here, or when they have "stayed the same" between renders.

The reference to the child component definitions are changing every time, and the data being passed in as props (at least they likely will be later). For minimal refactoring to improve this, I recommend the following making the following changes:

Create types.ts

export type FileInfo = { id: string; // ... } // Other types...

Create testdata.ts

import { FileInfo, // Other types... } from './types'; const files: FileInfo[] = [ { id: '1', title: 'my_file_name'.mp3, // ... }, // ... ]; const comments: Comment[] = [ ]; export const testData = { // this is shorthand for `files: files` files, comments, }

Then in App.tsx

import { FileInfo, Comment, } from './types'; import {testData} from './testdata'; function App() { return ( <div className="root"> <SectionTitle sectionData={testData.sectionData} /> <ChordProgression chordProgression={testData.chordProgression} /> <Files files={testData.files} /> <Comments comments={testData.comments} /> <Submit /> </div> ); } // Files component type FilesProps = { files: FileInfo } function Files(props: FileProps) { // then use `props.files` to render things }

We define components like Files outside of the definition of App. Ideally these are defined in their own files, but keeping them in the same file as App is less work for now. Separating the definitions to be on the module-level of the file is important though.

Okay, so do you want me to move the components to their own files?

Yeah each of the components seem separate enough to warrant living in their own individual files

Also vscode and typescript make it really easy to move components into other files. You can highlight the text of the component, then right-click and select "Refactor..." (I prefer to use the command palette and type "refactor" instead of right-clicking), then select "Move to a new file". It generates a new file, which you can then rename and relocate as necessary. vscode will automatically:

Move that component's function to a new file, and export it as a named export

Import that component in the original file

Move/copy and necessary imports (like React) into the new file, and remove them from this file if they are no longer used anywhere in this file

You don't have to do any of these things. It just does it for you. It's called "refactor" here since the change is made cleanly, and the functionality of the program doesn't change

Yeah man, I love the refactor command! It's so useful

src/App.tsx

mickmister · 2024-04-16T18:13:17Z

src/App.tsx

+  function Submit() {
+    return <div className="submit">
      <textarea placeholder="make a comment"></textarea>
      <button>Submit</button>
+    </div>


It probably makes sense to have the submit button trigger an update to the array of comments shown. That would make the code more ready for porting over.

We would add a prop to this Submit component for the parent component to process the form submission.

Also, I'm thinking this component can be called CreateComment to be more specific about what it's doing. We can then have the passed in callback prop to be called onSubmit. What do you think?

I think that renaming the component to CreateComment is a good idea for sure, since the name is now more clear as to its purpose.

I believe that in order for this component to update the array of comments shown, there are a few options we could go for, namely:

just adding a comment to the array (low complexity, but only a temporary soltion)

adding the array to local storage and actually updating the array there (a bit more involved but more of a long term solution)

What are your thoughts on this?

I'm thinking localStorage will be useful to help with mocking the actual backend now. Probably good idea to have a client.ts file that acts like we're fetching and saving to the backend. That will handle all localStorage things behind the scenes. Here, client refers to "http client" in a way. It's our gateway to the backend's endpoints, but we can fake the client's implementation with localStorage for now.

export const fetchAllDataForSectionPage = async (sectionId: string): Promise<SectionPageAllData> => { return getSectionDataFromLocalStorage(sectionId); }; export const addNewCommentToSection = async (sectionId: string, commentText: string): Promise<Comment> => { const sectionData = getSectionDataFromLocalStorage(sectionId); const id = Math.random().toString().slice(2); // generate random string const newComment: Comment = { id, message: commentText, }; sectionData.comments.push(comment); saveSectionDataToLocalStorage(sectionId, sectionData); return newComment; };

After this we can have a browse project page that will help show other aspects of the UI. It would be a view where you can select a given section in a project. Then we could also have a projects browser that allows you to see a list of projects. What do you think?

aelishRollo · 2024-04-27T19:45:18Z

src/Comments.tsx

@@ -0,0 +1,30 @@
+import * as Types from './types';


Currently when new comments are added, there are some rendering issues.

I think the CSS for this section needs to be made more elastic.

Let's go through this part in a pair programming session

I appreciate that! I actually already fixed it

aelishRollo · 2024-04-27T19:46:32Z

src/CreateComment.tsx

+    )
+  }
+
+  return (


We should add a <textarea> or here to capture the user's comment

aelishRollo · 2024-04-27T19:46:55Z

src/CreateComment.tsx

+
+  return (
+    <div>
+      <button onClick={() => handleAddComment({name: "New User", commentText: "This is a new comment."})}>


This data should come from the form, rather than being hardcoded like it is now

mickmister

Amazing work @aelishRollo! 🚀 🧪 💩

A few more comments for discussion. This is looking so good! I'd say let's get this to be more-or-less functional on a useState level, then we can move forward with other PRs for any backend stuff or other pages.

mickmister · 2024-04-28T01:14:08Z

src/App.tsx

+  function Submit() {
+    return <div className="submit">
      <textarea placeholder="make a comment"></textarea>
      <button>Submit</button>
+    </div>


I'm thinking localStorage will be useful to help with mocking the actual backend now. Probably good idea to have a client.ts file that acts like we're fetching and saving to the backend. That will handle all localStorage things behind the scenes. Here, client refers to "http client" in a way. It's our gateway to the backend's endpoints, but we can fake the client's implementation with localStorage for now.

export const fetchAllDataForSectionPage = async (sectionId: string): Promise<SectionPageAllData> => { return getSectionDataFromLocalStorage(sectionId); }; export const addNewCommentToSection = async (sectionId: string, commentText: string): Promise<Comment> => { const sectionData = getSectionDataFromLocalStorage(sectionId); const id = Math.random().toString().slice(2); // generate random string const newComment: Comment = { id, message: commentText, }; sectionData.comments.push(comment); saveSectionDataToLocalStorage(sectionId, sectionData); return newComment; };

After this we can have a browse project page that will help show other aspects of the UI. It would be a view where you can select a given section in a project. Then we could also have a projects browser that allows you to see a list of projects. What do you think?

mickmister · 2024-04-30T19:23:23Z

src/App.tsx

+import { Comments } from './Comments';
+import { CreateComment } from './CreateComment';
+import { SectionTitle } from './SectionTitle';
+import { useState } from 'react';


I'd like to add some linting to allow us to have a consistent ordering of imports. Then you can configure vscode to perform any lint fixes every time the file is saved locally

Okay, so what about integrating ESLint or Prettier in my local repo? That's one option, but I'm curious what approach you think is best

Yeah you can do it yourself as well. I would focus on eslint out of those two. I've been trying out biomejs but it's a bit too opinionated to me and doesn't allow for enough customization. Eslint supports all rules you could ever want, plus any styling that is supported by prettier for the most part. Eslint is king

How about we talk about this over video? I feel like there's a fair bit to unpack in this series of comments and I want to be sure we're on the same page

Here's the things I'm trying to figure out about this:

What’s Promise <SectionPageAllData ?

How do I know what the shape of this type will need to look like when I’m writing something similar in the future?

I guess the type here is for an object which contains all the data from localstorage/backend?

How are these two functions related? One seems to be for adding a comment and the other is for getting a file from localstorage. I get that they’re both useful but it feels kind of arbitrary to store them both together since they don’t seem related other than both making use of our mock backend. Maybe that’s the reason

The first function almost feels redundant. The function call it’s returning has essentially the same name as it. So what’s the point of it?

I’m not sure what you mean that client refers to http client. I know what a client is, but I’m not understanding the significance of this statement

I'd like to know more about what you have in mind for the project page with different views

I'm thinking about this, and I think that one large appeal of the code you wrote is that we're already giving things types such they could be async, despite localStorage being synchronous, so that when we go go to an actual backend we don't have to change them.

Would love to know your thoughts!

I think that one large appeal of the code you wrote is that we're already giving things types such they could be async, despite localStorage being synchronous, so that when we go go to an actual backend we don't have to change them.

@aelishRollo Yes that's correct 👍 You got it perfectly. We should "act" like we are working with a real backend as much as possible, including the naming and usage of functions being agnostic to which "backend" we are using, so we can then swap them out from each other. It may make even more sense to create an interface, and have a class LocalStorageBackendAPI that implements that interface.

As far as the term "backend", you could call the localStorage in this case a "storage backend", so it kind of already is a backend in itself.

mickmister · 2024-04-30T19:24:07Z

src/types.ts

+
+import React, { Dispatch } from "react"
+
+export type sectionData = {


Types are typically capitalized like so

Suggested change

export type sectionData = {

export type SectionData = {

mickmister · 2024-04-30T19:25:19Z

src/types.ts

+
+export type chordProgression = string[]
+
+export type chordProgressionProps = {


Props are typically defined right above the component that accepts those props, and are usually not exported either. Any parent component rendering this component infers the prop types automatically. Even though the type would not be exported, it implicitly is when calling/rendering the component since it accepts that as an argument

mickmister · 2024-04-30T19:26:04Z

src/testData.ts

+
+export const currentChordProgression: Types.chordProgression = ['C', 'Dm', 'F', 'G']
+
+export const files: Types.file[] = [


I think I would actually expect Types to be written as types. Namespaces like that are typically lowercased

mickmister · 2024-04-30T19:27:20Z

src/SectionTitle.tsx

@@ -0,0 +1,16 @@
+import * as Types from './types';
+
+export const SectionTitle: React.FC<Types.sectionDataProps> = ({ sectionData }) => {


We'll want this component to be editable by the user, so they can rename the section if desired

mickmister · 2024-04-30T19:28:15Z

src/Files.tsx

+      {files.map((file) => <div id={file.id}>
+        {file.title}


I would prefer this syntax in this case. What do you think?

Suggested change

{files.map((file) => <div id={file.id}>

{file.title}

{files.map((file) => (

<div id={file.id}>

{file.title}

Seems more like you're indenting it in a similar way to any other loop. It's also nice to return the JSX inside a set of parentheses like that. It seems nice and readable.

I like it and think we should use it

mickmister · 2024-04-30T19:28:55Z

src/Comments.tsx

+};
+
+
+
+//
+
+
+
+
+//


mickmister · 2024-04-30T19:29:07Z

src/Comments.tsx

+export const Comments: React.FC<Types.commentsProps> = ({ comments }) => {
+  return (
+    <div className="comments">
+      <span>3 Comments</span>


Suggested change

<span>3 Comments</span>

<span>{comments.length} Comments</span>

mickmister · 2024-04-30T19:34:31Z

src/Comments.tsx

@@ -0,0 +1,30 @@
+import * as Types from './types';


Let's go through this part in a pair programming session

mickmister · 2024-04-30T19:46:03Z

Leaving some notes here from a text conversation regarding backend design

I think we can avoid writing Go code by using Mattermost's API for data management. Any comments or files would go through that API. Project and music related data can live in an SQL database separate from Mattermost. We can handle shared authentication with supabase. Sign up for jam tools using supabase, get soundtown thrown in for free. And vise versa

So Mattermost channels would be an abstraction on the level of a project. A thread is any comment thread on any object that we want to support commenting on

Then channel membership governs file and comment access directly, for free. For making them completely public, the "public profile" thing is down the road where I'll visit that then

So we can have comments in "the database" for now, and for file uploads we use URLs for now. Data access control and permissions can be ignored at the moment, as that is all going to be handled by Mattermost. Everything is "public" atm since there will only be no authentication, until we set up supabase for the project. We should probably just set that up so we can get the database going

So Mattermost doesn't exist for now, as we will sort of mock its behavior by other means. Setting up supabase will get our production database up and be ready for authentication

You could then control data access through team and channel membership. And use public links to files for public uploads. Then fetch thread through a superuser or MM plugin

What would be even more lightweight would be to use localStorage, and have an import/export feature, to mimick storing in database and having data be portable

* create ci workflows for build and type checking * ignore empty dep array in useEffect * add linting

netlify · 2024-05-12T02:40:53Z

✅ Deploy Preview for section-view ready!

Name	Link
🔨 Latest commit	`bc5af78`
🔍 Latest deploy log	https://app.netlify.com/sites/section-view/deploys/665b7316b28a480008968330
😎 Deploy Preview	https://deploy-preview-4--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

This code is awesome 🎉 🤫 Coming along very well. Adding some more comments for discussion

I think setting up with a "backend" (localStorage being used as a key -> db table store), we can start building out a working prototype

mickmister · 2024-05-11T01:54:28Z

src/ChordProgression.tsx

+}
+
+
+export const ChordProgression: React.FC<ChordProgressionProps> = ({ chordProgression }) => {


Not necessarily a problem, but having two things named the same (this component and types.ChordProgression, which is exported as a named export in the types.ts file) can cause issues when you want to use your IDE to import one of those two things. I usually name this ChordProgressionView or something like that to avoid the name collision

Suggested change

export const ChordProgression: React.FC<ChordProgressionProps> = ({ chordProgression }) => {

export const ChordProgressionView: React.FC<ChordProgressionProps> = ({ chordProgression }) => {

mickmister · 2024-05-11T01:56:43Z

src/Comments.tsx

+
+    // Check if there are stored comments, otherwise use initial comments
+    setComments(storedComments || initialComments);
+  }, []); // Empty dependency array ensures this runs once on mount


Nice comment showing you know how this works. The comment itself is probably unnecessary for production code

Edit: This was signaling a linting error, so I disabled this line from the linting process

mickmister · 2024-05-11T02:00:57Z

src/Comments.tsx



-export const Comments: React.FC<Types.commentsProps> = ({ comments = initialComments }) => {
+export const Comments: React.FC<types.commentsProps> = ({ comments = initialComments, setComments }) => {


Nit: types.commentProps should be moved into this file as Props or CommentsProps

mickmister · 2024-05-11T03:18:19Z

src/Comments.tsx

+export const Comments: React.FC<types.commentsProps> = ({ comments = initialComments, setComments }) => {
+
+  useEffect(() => {
+    // Attempt to load comments from localStorage
+    const storedCommentsJSON = localStorage.getItem('comments');
+    const storedComments = storedCommentsJSON ? JSON.parse(storedCommentsJSON) : null;
+
+    // Check if there are stored comments, otherwise use initial comments
+    setComments(storedComments || initialComments);


Eventually there will be other places where comments will be rendered, with comments on other types of things even. I think this component should receive one of these as a prop:

comments - Array of Comment

entity - {entity_type: string, entity_id: string}; mentioned in Comments data model #5

I like the second one more. Then the Comments component is responsible for fetching from the store these comments.

Fetch all project data at page load from project id pointers. But lazy load comment bodies. Make use of "See comments" buttons at times to delay loading.

mickmister · 2024-05-12T02:45:02Z

src/CreateComment.tsx

+  const handleAddComment = () => {
+    const newComment = { name, commentText };
+
+    // Fetch existing comments from localStorage
+    const storedComments = localStorage.getItem('comments');
+    const existingComments = storedComments ? JSON.parse(storedComments) : [];
+
+    // Add the new comment to the array
+    const updatedComments = [...existingComments, newComment];
+
+    // Save the updated array back to localStorage
+    localStorage.setItem('comments', JSON.stringify(updatedComments));
+
+    // Update the local state to reflect the new list of comments
+    setComments(updatedComments);
  }


Swag 👍 🤫

mickmister · 2024-05-12T02:49:40Z

src/Files.tsx

+      {files.map((file) => (
+        <div id={file.id}>
        {file.title}
        <br></br> <br></br>
        {file.numComments + ' '}
        Comments
-      </div>)}
+      </div>))}


Indentation looks off here. Should be more like this:

{files.map((file) => ( <div id={file.id}> {file.title} <br></br> <br></br> {file.numComments + ' '} Comments </div> ))}

Also I tend to not use <br>, and instead do vertical spacing with css. Here I think I arbitrarily would create a file-title class that has margin-bottom

{files.map((file) => ( <div id={file.id}> <p className='file-title'>{file.title}</p> <p className='file-num-comments'> {file.numComments + ' '} Comments </p> </div> ))}

.file-title { margin-bottom: 30px; }

I set them to p, though span with display: inline-block would also work

I'm sure there's a more appropriate way to make that positioned that way vertically. Also we need to make sure this looks good on mobile

mickmister · 2024-05-12T02:51:13Z

src/SectionTitle.tsx

-        <button> Save revision </button>
+      <div className="section-title">
+          <div className='text'>
+              <h1>{currentTitle}</h1>


I have an idea of user experience of how we can edit the title. I think we should be able to click the current title, or an edit button next to it, and then the title becomes an input box where you can edit its content. Then the submit button changes back to an h1 element

For an edit button, we can use the pencil icon from FontAwesome

https://fontawesome.com/icons/pencil?f=classic&s=solid
https://fontawesome.com/search

mickmister · 2024-05-12T02:52:23Z

src/SectionTitle.tsx

+      const formData = new FormData(event.currentTarget); // Using event.currentTarget to reference the form
+      const newName = formData.get('newName') as string; 
+      setCurrentTitle(newName); 
+      setShowForm(false);


Interesting to get the data from the form this way. Any particular reason you preferred this over using useState etc.?

mickmister · 2024-05-12T03:05:34Z

src/Comments.tsx

+import { useEffect} from 'react';
+
+
+export const Comments: React.FC<types.commentsProps> = ({ comments = initialComments, setComments }) => {


It may make sense to create a src/components folder to put components like this

* initial take on refactoring to store and client * connect section title to store * make the app use localStorage * remote IStore interface to simplify. fix workflow test command * use typed in username for comments * add import for index.css

aelishRollo added 10 commits April 12, 2024 11:27

Refactor CSS reset to its own file. Standardize naming conventions

3ecc9ef

Change .parent to .root

b947804

Make sectionData, currentChordProgression, files, and comments, data …

5119278

…driven

Refactor files to be its own component

ad77205

Refactor ChordProgression into a component

69a671d

Refactor Chordprogression for to include the <div> with class chords

627dc8f

Refactor Files so <div> with class 'files' is all together

233b7e8

Refactor comments section into its own component

eb84b11

Refactor submit section into its own component

812d456

Refactor Section-Title into its own component

3b2d7e2

mickmister commented Apr 15, 2024

View reviewed changes

src/App.tsx Outdated Show resolved Hide resolved

mickmister commented Apr 16, 2024

View reviewed changes

aelishRollo added 17 commits April 20, 2024 14:14

Give all component return statements pretty parentheses

b0cd8b4

Refactor sectionData to be passed in as a prop

aaeaec0

Refactor chord progression component to use props

cf1bde7

Refactor files component to use props

660977c

Refactor Comments to use props

b494a52

Add an id for the array of files

4dbcfec

Move type definitions to Types.ts

22e7c24

Add types to testData.ts

ae941bd

Remove unnecessary import

11c9a6e

Add export keywords to testData.ts

ca323bb

Move all data to testData.tsx

72db333

Refactor App.tsx to accept testData as a prop

631ef8a

Prepare to refactor components to their own files

4a263fc

Refactor components to their own files

41a8b2e

Add comments as state variable, hoist it

8855891

Remove extra map from .submit

7620d10

Rename Submit component to CreateComment

ac1ea6c

aelishRollo reviewed Apr 27, 2024

View reviewed changes

aelishRollo added 3 commits April 30, 2024 10:12

Fix comment update bug

b06a3d4

Remove unnecessary imports

4fa90a9

Fix overflow issue when adding a new comment

763e61f

mickmister commented Apr 30, 2024

View reviewed changes

aelishRollo and others added 16 commits May 1, 2024 10:18

Make all types start with uppercase character

948eaab

Move AppProps to App.jsx

8413354

Move CreateCommentProps to CreateComment.tsx

b18ed6a

Move FilesProps definition to Files.tsx

b0b6b80

Move ChordProgressionProps to ChordProgression.tsx

9390c2d

Move SectionDataProps to SectionData.tsx

b9c09d0

Change 'Types' to 'types'

4b48b2c

Change '.buttons' to '.revisions'

5842ae0

User can now rename section title

b949d42

Add parenthese to returned JSX

7676ef0

Make amount of comments show on page

70206b1

Prep to move to localStorage

dff866e

Make comments persistant using localStorage

371de77

Rename testData.tsx to sampleData.tsx

2b25ef6

Move comments data to localStorage

8efdec6

Create ci workflows for build and type checking (#7)

524408f

* create ci workflows for build and type checking * ignore empty dep array in useEffect * add linting

mickmister commented May 12, 2024

View reviewed changes

aelishRollo and others added 2 commits May 24, 2024 16:18

Update README.md

571c1b0

Refactor to use local storage (#8)

bc5af78

* initial take on refactoring to store and client * connect section title to store * make the app use localStorage * remote IStore interface to simplify. fix workflow test command * use typed in username for comments * add import for index.css


		import React, { Dispatch } from "react"

		export type sectionData = {


		export type chordProgression = string[]

		export type chordProgressionProps = {


		export const currentChordProgression: Types.chordProgression = ['C', 'Dm', 'F', 'G']

		export const files: Types.file[] = [

		@@ -0,0 +1,16 @@
		import * as Types from './types';

		export const SectionTitle: React.FC<Types.sectionDataProps> = ({ sectionData }) => {

+              };
+              //
+              //

	<span>3 Comments</span>
	<span>{comments.length} Comments</span>

		}


		export const ChordProgression: React.FC<ChordProgressionProps> = ({ chordProgression }) => {

	export const ChordProgression: React.FC<ChordProgressionProps> = ({ chordProgression }) => {
	export const ChordProgressionView: React.FC<ChordProgressionProps> = ({ chordProgression }) => {



		export const Comments: React.FC<Types.commentsProps> = ({ comments = initialComments }) => {
		export const Comments: React.FC<types.commentsProps> = ({ comments = initialComments, setComments }) => {

		import { useEffect} from 'react';


		export const Comments: React.FC<types.commentsProps> = ({ comments = initialComments, setComments }) => {

Compare since previous compare PR #4

Are you sure you want to change the base?

Compare since previous compare PR #4

Conversation

mickmister commented Apr 15, 2024 • edited Loading

mickmister left a 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

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 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aelishRollo May 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aelishRollo May 3, 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

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 commented Apr 30, 2024

netlify bot commented May 12, 2024 • edited Loading

✅ Deploy Preview for section-view ready!

mickmister left a 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

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 commented Apr 15, 2024 •

edited

Loading

aelishRollo May 1, 2024 •

edited

Loading

aelishRollo May 3, 2024 •

edited

Loading

netlify bot commented May 12, 2024 •

edited

Loading