-
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 46 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 |
---|---|---|
@@ -0,0 +1,52 @@ | ||
name: CI | ||
|
||
on: | ||
push: | ||
branches: | ||
- main | ||
pull_request: | ||
branches: | ||
- '**' | ||
workflow_dispatch: | ||
|
||
jobs: | ||
build: | ||
runs-on: ubuntu-latest | ||
steps: | ||
- uses: actions/checkout@v3 | ||
- name: Install Node.js | ||
uses: actions/setup-node@v4 | ||
with: | ||
node-version: 21 | ||
cache: 'npm' | ||
- name: Install modules | ||
run: npm i | ||
- name: Build app | ||
run: npm run build | ||
types: | ||
runs-on: ubuntu-latest | ||
steps: | ||
- uses: actions/checkout@v3 | ||
- name: Install Node.js | ||
uses: actions/setup-node@v4 | ||
with: | ||
node-version: 21 | ||
cache: 'npm' | ||
- name: Install modules | ||
run: npm i | ||
- name: Check Types | ||
run: npm run check-types | ||
|
||
lint: | ||
runs-on: ubuntu-latest | ||
steps: | ||
- uses: actions/checkout@v3 | ||
- name: Install Node.js | ||
uses: actions/setup-node@v4 | ||
with: | ||
node-version: 21 | ||
cache: 'npm' | ||
- name: Install modules | ||
run: npm i | ||
- name: Run eslint | ||
run: npm run lint |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -21,3 +21,5 @@ | |
npm-debug.log* | ||
yarn-debug.log* | ||
yarn-error.log* | ||
|
||
_ignore |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
tasks: | ||
- init: npm install && npm run build | ||
command: npm run start |
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 './sampleData' | ||
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,16 @@ | ||||||
import * as types from './types'; | ||||||
|
||||||
type ChordProgressionProps = { | ||||||
chordProgression: types.ChordProgression | ||||||
} | ||||||
|
||||||
|
||||||
export const ChordProgression: React.FC<ChordProgressionProps> = ({ chordProgression }) => { | ||||||
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. Not necessarily a problem, but having two things named the same (this component and
Suggested change
|
||||||
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,33 @@ | ||
import * as types from './types'; | ||
import { FontAwesomeIcon } from '@fortawesome/react-fontawesome'; | ||
import { faFaceSmile } from '@fortawesome/free-solid-svg-icons'; | ||
import {comments as initialComments} from './sampleData' | ||
import { useEffect} from 'react'; | ||
|
||
|
||
export const Comments: React.FC<types.commentsProps> = ({ comments = initialComments, setComments }) => { | ||
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. Nit: 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. It may make sense to create a |
||
|
||
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); | ||
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. 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:
I like the second one more. Then the 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. |
||
// eslint-disable-next-line react-hooks/exhaustive-deps | ||
}, []); | ||
|
||
|
||
return ( | ||
<div className="comments"> | ||
<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> | ||
); | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,59 @@ | ||
|
||
import {Comment} from './types'; | ||
import {useState} from 'react'; | ||
|
||
|
||
type CreateCommentProps = { | ||
comments: Comment[], | ||
setComments: (comments: Comment[]) => void; | ||
} | ||
|
||
|
||
|
||
export const CreateComment: React.FC<CreateCommentProps> = ({ comments, setComments }) => { | ||
|
||
const [name, setName] = useState(''); | ||
const [commentText, setCommentText] = useState(''); | ||
|
||
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); | ||
} | ||
|
||
|
||
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> | ||
<form onSubmit={(e) => { | ||
e.preventDefault(); | ||
handleAddComment(); | ||
setCommentText(''); | ||
}}> | ||
<input | ||
type='text' | ||
value={name} | ||
onChange={(e) => setName(e.target.value)} | ||
placeholder='Enter your name' | ||
/> | ||
<input | ||
type='text' | ||
value={commentText} | ||
onChange={(e) => setCommentText(e.target.value)} | ||
placeholder='Type your thoughts here' | ||
/> | ||
<button type='submit'>Add Comment</button> | ||
</form> | ||
</div> | ||
); | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,20 @@ | ||
import * as types from './types'; | ||
|
||
type FilesProps = { | ||
files: types.File[] | ||
} | ||
|
||
export const Files: React.FC<FilesProps> = ({ files }) => { | ||
return ( | ||
<div className="files"> | ||
<span>+ Files</span> | ||
{files.map((file) => ( | ||
<div id={file.id}> | ||
{file.title} | ||
<br></br> <br></br> | ||
{file.numComments + ' '} | ||
Comments | ||
</div>))} | ||
</div> | ||
); | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,51 @@ | ||
import * as types from './types'; | ||
import React, { useState, FormEvent } from 'react' | ||
|
||
|
||
|
||
|
||
type SectionDataProps = { | ||
sectionData: types.SectionData, | ||
} | ||
|
||
export const SectionTitle: React.FC<SectionDataProps> = ({ sectionData }) => { | ||
const [showForm, setShowForm] = useState(false); //shows the form for when you're entering a new title | ||
const [currentTitle, setCurrentTitle] = useState(sectionData.name); | ||
|
||
let handleToggleForm = () => { | ||
console.log('button clicked'); | ||
setShowForm(!showForm); | ||
}; | ||
|
||
// Handle form submission with FormData for TypeScript | ||
const handleSubmit = (event: FormEvent<HTMLFormElement>) => { | ||
event.preventDefault(); | ||
const formData = new FormData(event.currentTarget); // Using event.currentTarget to reference the form | ||
const newName = formData.get('newName') as string; | ||
setCurrentTitle(newName); | ||
setShowForm(false); | ||
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. Interesting to get the data from the form this way. Any particular reason you preferred this over using |
||
}; | ||
|
||
return ( | ||
<div className="section-title"> | ||
<div className='text'> | ||
<h1>{currentTitle}</h1> | ||
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 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 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. For an edit button, we can use the pencil icon from FontAwesome https://fontawesome.com/icons/pencil?f=classic&s=solid |
||
<p>{sectionData.description}</p> | ||
<button onClick={handleToggleForm}> | ||
{showForm ? 'Cancel Editing' : 'Rename Section'} | ||
</button> | ||
</div> | ||
{showForm && ( | ||
<form onSubmit={handleSubmit}> | ||
<label htmlFor="newName">New Name:</label> | ||
<input id="newName" name="newName" type="text" placeholder="Enter new section name" /> | ||
<button type="submit">Update Name</button> | ||
</form> | ||
)} | ||
<div className='revisions'> | ||
<button>{sectionData.numRevisions} revisions</button> | ||
<button>Save revision</button> | ||
</div> | ||
</div> | ||
); | ||
}; |
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.