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

Compare since previous compare PR #4

Open
wants to merge 48 commits into
base: initial
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 27 commits
Commits
Show all changes
48 commits
Select commit Hold shift + click to select a range
3ecc9ef
Refactor CSS reset to its own file. Standardize naming conventions
aelishRollo Apr 12, 2024
b947804
Change .parent to .root
aelishRollo Apr 12, 2024
5119278
Make sectionData, currentChordProgression, files, and comments, data …
aelishRollo Apr 12, 2024
ad77205
Refactor files to be its own component
aelishRollo Apr 13, 2024
69a671d
Refactor ChordProgression into a component
aelishRollo Apr 13, 2024
627dc8f
Refactor Chordprogression for to include the <div> with class chords
aelishRollo Apr 13, 2024
233b7e8
Refactor Files so <div> with class 'files' is all together
aelishRollo Apr 13, 2024
eb84b11
Refactor comments section into its own component
aelishRollo Apr 13, 2024
812d456
Refactor submit section into its own component
aelishRollo Apr 13, 2024
3b2d7e2
Refactor Section-Title into its own component
aelishRollo Apr 13, 2024
b0cd8b4
Give all component return statements pretty parentheses
aelishRollo Apr 20, 2024
aaeaec0
Refactor sectionData to be passed in as a prop
aelishRollo Apr 22, 2024
cf1bde7
Refactor chord progression component to use props
aelishRollo Apr 23, 2024
660977c
Refactor files component to use props
aelishRollo Apr 23, 2024
b494a52
Refactor Comments to use props
aelishRollo Apr 23, 2024
4dbcfec
Add an id for the array of files
aelishRollo Apr 24, 2024
22e7c24
Move type definitions to Types.ts
aelishRollo Apr 24, 2024
ae941bd
Add types to testData.ts
aelishRollo Apr 25, 2024
11c9a6e
Remove unnecessary import
aelishRollo Apr 25, 2024
ca323bb
Add export keywords to testData.ts
aelishRollo Apr 25, 2024
72db333
Move all data to testData.tsx
aelishRollo Apr 25, 2024
631ef8a
Refactor App.tsx to accept testData as a prop
aelishRollo Apr 26, 2024
4a263fc
Prepare to refactor components to their own files
aelishRollo Apr 26, 2024
41a8b2e
Refactor components to their own files
aelishRollo Apr 26, 2024
8855891
Add comments as state variable, hoist it
aelishRollo Apr 27, 2024
7620d10
Remove extra map from .submit
aelishRollo Apr 27, 2024
ac1ea6c
Rename Submit component to CreateComment
aelishRollo Apr 27, 2024
b06a3d4
Fix comment update bug
aelishRollo Apr 30, 2024
4fa90a9
Remove unnecessary imports
aelishRollo Apr 30, 2024
763e61f
Fix overflow issue when adding a new comment
aelishRollo Apr 30, 2024
948eaab
Make all types start with uppercase character
aelishRollo May 1, 2024
8413354
Move AppProps to App.jsx
aelishRollo May 1, 2024
b18ed6a
Move CreateCommentProps to CreateComment.tsx
aelishRollo May 1, 2024
b0b6b80
Move FilesProps definition to Files.tsx
aelishRollo May 1, 2024
9390c2d
Move ChordProgressionProps to ChordProgression.tsx
aelishRollo May 1, 2024
b9c09d0
Move SectionDataProps to SectionData.tsx
aelishRollo May 1, 2024
4b48b2c
Change 'Types' to 'types'
aelishRollo May 1, 2024
5842ae0
Change '.buttons' to '.revisions'
aelishRollo May 2, 2024
b949d42
User can now rename section title
aelishRollo May 3, 2024
7676ef0
Add parenthese to returned JSX
aelishRollo May 3, 2024
70206b1
Make amount of comments show on page
aelishRollo May 3, 2024
dff866e
Prep to move to localStorage
aelishRollo May 7, 2024
371de77
Make comments persistant using localStorage
aelishRollo May 8, 2024
2b25ef6
Rename testData.tsx to sampleData.tsx
aelishRollo May 8, 2024
8efdec6
Move comments data to localStorage
aelishRollo May 9, 2024
524408f
Create ci workflows for build and type checking (#7)
mickmister May 12, 2024
571c1b0
Update README.md
aelishRollo May 24, 2024
bc5af78
Refactor to use local storage (#8)
mickmister Jun 1, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 9 additions & 2 deletions src/App.test.tsx
Original file line number Diff line number Diff line change
@@ -1,9 +1,16 @@
import React from 'react';
import { render, screen } from '@testing-library/react';
import * as testData from './testData'
import App from './App';

test('renders learn react link', () => {
render(<App />);
render(
<App
sectionData={testData.sectionData}
chordProgression={testData.currentChordProgression}
files={testData.files}
comments={testData.comments}
/>
);
const linkElement = screen.getByText(/learn react/i);
expect(linkElement).toBeInTheDocument();
});
63 changes: 21 additions & 42 deletions src/App.tsx
Original file line number Diff line number Diff line change
@@ -1,48 +1,27 @@
import './App.css';
import './SectionView.css';
import { FontAwesomeIcon } from '@fortawesome/react-fontawesome';
import { faFaceSmile } from '@fortawesome/free-solid-svg-icons';
import './css_reset.css'
import './section_view.css';
import * as Types from './types';
import { Files } from './Files';
import { ChordProgression } from './ChordProgression';
import { Comments } from './Comments';
import { CreateComment } from './CreateComment';
import { SectionTitle } from './SectionTitle';
import { useState } from 'react';
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'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

Copy link
Member

@aelishRollo aelishRollo May 1, 2024

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

@aelishRollo aelishRollo May 3, 2024

Choose a reason for hiding this comment

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

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

  1. 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?

  1. 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

  2. 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?

  3. 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

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

Copy link
Member

Choose a reason for hiding this comment

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

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!

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 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.




const App:React.FC<Types.appProps> = ({sectionData, chordProgression, comments, files}) => {

const [commentsAsState, setCommentsAsState] = useState<Types.comment[]>(comments)

function App() {
return (
<div className="parent">
<div className="section-title">
<div className='text'>
<h1> Intro </h1>
<p>This intro section consists of a tuba quartet in the style of DJ Templeton & The Windsurfers</p>
</div>
<div className='buttons'>
<button> 42 revisions </button>
<button> save revision </button>
</div>
</div>
<div className="chords">
<ol>
<li>C</li>
<li>Dm</li>
<li>F</li>
<li>G</li>
</ol>
</div>
<div className="files">
<span>+ Files</span>
<div>Bass.mp3 <br></br> <br></br> 2 Comments</div>
<div>Drums.mp3 <br></br> <br></br> 2 Comments</div>
<div>Yodeling.mp3 <br></br> <br></br> 2 Comments </div>
<div>Tuba.mp3 <br></br> <br></br> 2 Comments </div>
</div>
<div className="comments">
<span>3 Comments</span>
<div className="display-comments">
<p><FontAwesomeIcon icon={faFaceSmile} /> Jerry: My name is Schmoopie and I love this song. It reminds me of my grandpa</p>
<p><FontAwesomeIcon icon={faFaceSmile} /> Larry: jokes on you! Im deaf.</p>
<p><FontAwesomeIcon icon={faFaceSmile} /> Terry: Notice the tinnitus ringing in your ears, let the hiss from your damaged hearing remind you of peaceful ocean waves...</p>
</div>
</div>
<div className="submit">
<textarea placeholder="make a comment"></textarea>
<button>Submit</button>
</div>
<div className="root">
<SectionTitle sectionData={sectionData} />
<ChordProgression chordProgression={chordProgression} />
<Files files={files}/>
<Comments comments={commentsAsState}/>
<CreateComment comments={commentsAsState} setComments={setCommentsAsState}/>
</div>
);
}
Expand Down
11 changes: 11 additions & 0 deletions src/ChordProgression.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
import * as Types from './types';

export const ChordProgression: React.FC<Types.chordProgressionProps> = ({ chordProgression }) => {
return (
<div className="chords">
<ol>
{chordProgression.map((chord, index) => <li>{chord}</li>)}
</ol>
</div>
);
};
30 changes: 30 additions & 0 deletions src/Comments.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
import * as Types from './types';
Copy link
Member

Choose a reason for hiding this comment

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

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

I appreciate that! I actually already fixed it

import { FontAwesomeIcon } from '@fortawesome/react-fontawesome';
import { faFaceSmile } from '@fortawesome/free-solid-svg-icons';
import {useState} from 'react'
import {comments as initialComments} from './testData'
import { comment } from './types';

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

Choose a reason for hiding this comment

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

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

<div className="display-comments">
{comments.map((comment, index) => {
return <>
<p><FontAwesomeIcon icon={faFaceSmile} /> {comments[index].name}: {comments[index].commentText}</p>
</>;
})}
</div>
</div>
);
};



//




//
Copy link
Member Author

Choose a reason for hiding this comment

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

Nice

21 changes: 21 additions & 0 deletions src/CreateComment.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
import {useState} from 'react'
import {comments as initialComments} from './testData'
import { CreateCommentProps, comment, commentsProps } from './types';

export const CreateComment:React.FC<CreateCommentProps> = ({comments, setComments}) => {
// const [comments, setComment] = useState(initialComments)

const handleAddComment = (newComment:comment) => {
setComments(
comments => [...comments, newComment]
)
}

return (
Copy link
Member

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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

Add Comment
</button>
</div>
);
};
15 changes: 15 additions & 0 deletions src/Files.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
import * as Types from './types';

export const Files: React.FC<Types.filesProps> = ({ files }) => {
return (
<div className="files">
<span>+ Files</span>
{files.map((file) => <div id={file.id}>
{file.title}
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 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}

Copy link
Member

Choose a reason for hiding this comment

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

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

<br></br> <br></br>
{file.numComments + ' '}
Comments
</div>)}
</div>
);
};
16 changes: 16 additions & 0 deletions src/SectionTitle.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
import * as Types from './types';

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

Choose a reason for hiding this comment

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

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

return (
<div className="section-title">
<div className='text'>
<h1> {sectionData.name} </h1>
<p>{sectionData.description}</p>
</div>
<div className='buttons'>
<button> {sectionData.numRevisions} revisions </button>
<button> Save revision </button>
</div>
</div>
);
};
48 changes: 48 additions & 0 deletions src/css_reset.css
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
/* http://meyerweb.com/eric/tools/css/reset/
v2.0 | 20110126
License: none (public domain)
*/

html, body, div, span, applet, object, iframe,
h1, h2, h3, h4, h5, h6, p, blockquote, pre,
a, abbr, acronym, address, big, cite, code,
del, dfn, em, img, ins, kbd, q, s, samp,
small, strike, strong, sub, sup, tt, var,
b, u, i, center,
dl, dt, dd, ol, ul, li,
fieldset, form, label, legend,
table, caption, tbody, tfoot, thead, tr, th, td,
article, aside, canvas, details, embed,
figure, figcaption, footer, header, hgroup,
menu, nav, output, ruby, section, summary,
time, mark, audio, video {
margin: 0;
padding: 0;
border: 0;
font-size: 100%;
font: inherit;
vertical-align: baseline;
}
/* HTML5 display-role reset for older browsers */
article, aside, details, figcaption, figure,
footer, header, hgroup, menu, nav, section {
display: block;
}
body {
line-height: 1;
}
ol, ul {
list-style: none;
}
blockquote, q {
quotes: none;
}
blockquote:before, blockquote:after,
q:before, q:after {
content: '';
content: none;
}
table {
border-collapse: collapse;
border-spacing: 0;
}
8 changes: 7 additions & 1 deletion src/index.tsx
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import React from 'react';
import ReactDOM from 'react-dom/client';
import './index.css';
import * as testData from './testData'
import App from './App';
import reportWebVitals from './reportWebVitals';

Expand All @@ -9,7 +10,12 @@ const root = ReactDOM.createRoot(
);
root.render(
<React.StrictMode>
<App />
<App
sectionData={testData.sectionData}
chordProgression={testData.currentChordProgression}
files={testData.files}
comments={testData.comments}
/>
</React.StrictMode>
);

Expand Down
62 changes: 1 addition & 61 deletions src/SectionView.css → src/section_view.css
Original file line number Diff line number Diff line change
@@ -1,63 +1,3 @@
/* http://meyerweb.com/eric/tools/css/reset/
v2.0 | 20110126
License: none (public domain)
*/

html, body, div, span, applet, object, iframe,
h1, h2, h3, h4, h5, h6, p, blockquote, pre,
a, abbr, acronym, address, big, cite, code,
del, dfn, em, img, ins, kbd, q, s, samp,
small, strike, strong, sub, sup, tt, var,
b, u, i, center,
dl, dt, dd, ol, ul, li,
fieldset, form, label, legend,
table, caption, tbody, tfoot, thead, tr, th, td,
article, aside, canvas, details, embed,
figure, figcaption, footer, header, hgroup,
menu, nav, output, ruby, section, summary,
time, mark, audio, video {
margin: 0;
padding: 0;
border: 0;
font-size: 100%;
font: inherit;
vertical-align: baseline;
}
/* HTML5 display-role reset for older browsers */
article, aside, details, figcaption, figure,
footer, header, hgroup, menu, nav, section {
display: block;
}
body {
line-height: 1;
}
ol, ul {
list-style: none;
}
blockquote, q {
quotes: none;
}
blockquote:before, blockquote:after,
q:before, q:after {
content: '';
content: none;
}
table {
border-collapse: collapse;
border-spacing: 0;
}









/* END OF CSS RESET */




* {
Expand All @@ -67,7 +7,7 @@ table {
html {font-size: 62.5%;}


.parent {
.root {
display: flex;
background-color: rgb(95, 193, 208);
justify-content: center;
Expand Down
49 changes: 49 additions & 0 deletions src/testData.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@

import * as Types from './types'

export const sectionData: Types.sectionData = {
name: 'Intro',
description: 'This intro section consists of a tuba quartet in the style of DJ Templeton & The Windsurfers',
numRevisions: 42,
};

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

export const files: Types.file[] = [
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 think I would actually expect Types to be written as types. Namespaces like that are typically lowercased

{
title: 'Bass.mp3',
numComments: 2,
id: 'change me to something better 0 '
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 think just regular 0 is fine here for now. It's somewhat realistic I think. You can use this to create a random string in the meantime:

Suggested change
id: 'change me to something better 0 '
id: Math.random().toString().slice(2),

},
{
title: 'Drums.mp3',
numComments: 2,
id: 'change me to something better 1'
},
{
title: 'Yodeling.mp3',
numComments: 2,
id: 'change me to something better 2'
},
{
title: 'Tuba.mp3',
numComments: 2,
id: 'change me to something better 3'
},
]


export const comments: Types.comment[] = [
{
name: 'Jerry',
commentText: 'My name is Schmoopie and I love this song. It reminds me of my grandpa'
},
{
name: 'Lerry',
commentText: 'jokes on you! Im deaf.'
},
{
name: 'Jerry',
commentText: 'Notice the tinnitus ringing in your ears, let the hiss from your damaged hearing remind you of peaceful ocean waves...'
},
]
Loading