-
Notifications
You must be signed in to change notification settings - Fork 0
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
base: initial
Are you sure you want to change the base?
Changes from 17 commits
3ecc9ef
b947804
5119278
ad77205
69a671d
627dc8f
233b7e8
eb84b11
812d456
3b2d7e2
b0cd8b4
aaeaec0
cf1bde7
660977c
b494a52
4dbcfec
22e7c24
ae941bd
11c9a6e
ca323bb
72db333
631ef8a
4a263fc
41a8b2e
8855891
7620d10
ac1ea6c
b06a3d4
4fa90a9
763e61f
948eaab
8413354
b18ed6a
b0b6b80
9390c2d
b9c09d0
4b48b2c
5842ae0
b949d42
7676ef0
70206b1
dff866e
371de77
2b25ef6
8efdec6
524408f
571c1b0
bc5af78
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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(); | ||
}); |
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> | ||
); | ||
}; |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,30 @@ | ||||||
import * as Types from './types'; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's go through this part in a pair programming session There was a problem hiding this comment. Choose a reason for hiding this commentThe 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> | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
<div className="display-comments"> | ||||||
{comments.map((comment, index) => { | ||||||
return <> | ||||||
<p><FontAwesomeIcon icon={faFaceSmile} /> {comments[index].name}: {comments[index].commentText}</p> | ||||||
</>; | ||||||
})} | ||||||
</div> | ||||||
</div> | ||||||
); | ||||||
}; | ||||||
|
||||||
|
||||||
|
||||||
// | ||||||
|
||||||
|
||||||
|
||||||
|
||||||
// | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice |
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 ( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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."})}> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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> | ||
); | ||
}; |
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} | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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> | ||||||||||||
); | ||||||||||||
}; |
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 }) => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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> | ||
); | ||
}; |
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[] = [ | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think I would actually expect |
||||||
{ | ||||||
title: 'Bass.mp3', | ||||||
numComments: 2, | ||||||
id: 'change me to something better 0 ' | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think just regular
Suggested change
|
||||||
}, | ||||||
{ | ||||||
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...' | ||||||
}, | ||||||
] |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,57 @@ | ||||||
|
||||||
import React, { Dispatch } from "react" | ||||||
|
||||||
export type sectionData = { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Types are typically capitalized like so
Suggested change
|
||||||
name: string, | ||||||
description: string, | ||||||
numRevisions: number | ||||||
} | ||||||
|
||||||
export type sectionDataProps = { | ||||||
sectionData: sectionData | ||||||
} | ||||||
|
||||||
|
||||||
|
||||||
export type chordProgression = string[] | ||||||
|
||||||
export type chordProgressionProps = { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||||||
chordProgression: chordProgression | ||||||
} | ||||||
|
||||||
|
||||||
|
||||||
export type file = { | ||||||
title: string, | ||||||
numComments: number, | ||||||
id: string, | ||||||
} | ||||||
|
||||||
export type filesProps = { | ||||||
files: file[] | ||||||
} | ||||||
|
||||||
|
||||||
export type comment = { | ||||||
name: string, | ||||||
commentText: string | ||||||
} | ||||||
|
||||||
export type commentsProps = { | ||||||
comments: comment[] | ||||||
} | ||||||
|
||||||
|
||||||
|
||||||
export type CreateCommentProps = { | ||||||
comments: comment[], | ||||||
setComments: React.Dispatch<(comments: comment[]) => comment[]> | ||||||
} | ||||||
|
||||||
|
||||||
export type appProps = { | ||||||
sectionData: sectionData, | ||||||
chordProgression: chordProgression, | ||||||
files: file[], | ||||||
comments: comment[] | ||||||
} |
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.
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
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.
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
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.
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
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.
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
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.
Here's the things I'm trying to figure out about this:
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
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.
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!
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.
@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 aclass 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.