Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Hetav Desai | FrontEnd Project 1 Submission #1
base: main
Are you sure you want to change the base?
Hetav Desai | FrontEnd Project 1 Submission #1
Changes from 16 commits
44af1d0
bff66aa
bd97b22
9dc4100
da751ed
ae00045
5430335
ab55b63
f5b6eed
6910646
8e9d255
cedfbb4
4708be6
e82142b
74dc89c
bd67c35
bbbb7de
281e39d
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
instead of parsing twice, you can parse once and store it in a variable. Also, on first start
notesArr
would be undefined, and hence JSON.parse will fail with error, won't it?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.
Yup, using variable would be better.
And
localStorage.getItem()
returns null if the data we want isn't store in local storage. And hence, on first start,notesArr
will be null andJSON.parse()
will return null as well.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.
You have already stored this DOM element at the top. You can use that?
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.
Turns out that I was trying to store a few elements like this one even before they were rendered and hence they would return null when trying to store on top. Got rid of such statements on top and used them where they were required, after they had been rendered.
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.
Doesn't seems like it renders selectednote, it renders the note whose index is passed. So may be
renderNote
orrenderNoteWithIndex
.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.
So here I'm always passing the index of selected note. But yeah,
renderNoteWithIndex
makes more sense.Also, I could get rid of accepting index in this function and just read the selectedNoteIndex from the global scope.
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.
also, a convention to name booleans is that they start with is, should, has etc...to sound like a question.
Eg. hasItems, shouldOpen, isEmpty....
@desaihetav now you suggest me a better name for
clearEmpty
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.
Yes, got the point. So this can be better named as
shouldClearEmpty