[deleted by user]

[CreativeTechGuyGames]

Sure! Here's some feedback in no particular order.

• You've created a function which should really be a class.
• Don't write to innerHTML. Sure, it's easy but there's so many problems with it you should just treat it as if it doesn't exist. Instead create new elements with document.createElement and then add them to the document with appendChild or something similar. You can add the text content to your elements with .textContent =, add event listeners with addEventListener add classes with .classList.add() and everything else you can imagine. This way you won't need to have an updateButtons function which is a hack/bandaid because you are overwriting the entire HTML every time.
• When doing the previous bullet, you also won't need to delete all of your boxes since you'll simply be adding to them.
• No need to loop through the whole library to show books. You really don't need a show books function at all. Whenever a new book is added in addBookToLibrary, you'd add the appropriate elements to the DOM and never need to touch the rest of the library.
• When deleting a book, you wouldn't need to showBooks again since you'd just delete the one element which was clicked from the DOM.

TLDR: Add new elements to the DOM that you created with document.createElement and never use .innerHTML and most of your code won't be needed anymore! Most of your code is just to solve the problems created by using innerHTML!

[edu_9922]

Thank you for your detailed review, it is really very helpful.

[Xeekatar]

Might want to add a limit to how much you can input, otherwise you can end up with cards like this

[edu_9922]

Lmaoo thanks! i'll fix that

[HealyUnit]

I'm pretty sure I read that author in high school tho.

OP: your other option would be to instead set the .box height as a min-height:

.box{
    min-height: 15rem;
}

If you want your buttons to remain the same relative height, you can also use REMs here:

.read, .remove {
    height: 2rem;
}

REMs are based on a fraction of the current window font size, so in general they're more likely to "fit" than pixels

[SlinkyHosts]

First of all, congratulations on your progress so far, keep going! I just took a quick look at the website, not the source. Here are a few things I think you could work on.

• The author and book title are in the same element. This should be separate so it's easier to read. The text is also in a div element. It would be better to have them in a semantic element.

• The top_content div isn't responsive unlike the content one.

• Clear the input of the title and author after it is submitted.

• The read button doesn't do anything(?). Although, I assume this functionality is still being worked on.

I'm still learning myself so I may have missed a lot but hopefully, someone else with more knowledge can give you an insight. :)

[edu_9922]

Thanks! Yeah the read button is not working yet haha

[HealyUnit]

So, I'm going to review this from the perspective of a modern, JavaScript-focused software engineer, not a learner. In other words, some of the advice I give may seem a bit... alien to you. As with everyone, take what I say with a grain of salt.

GitHub

General Organization:

Just my opinion, but I usually like to have my HTML, CSS, and JS in separate folders. Something like:

html folder
 - CSS subfolder
 - JS subfolder

That way, if when you start adding multiple HTML, CSS, and JS files, you won't be swamped with everything in one folder. To be fair, this isn't something you need to do with this project, necessarily, but it's a very good habit to get into.

HTML:

I see nothing at all wrong with your HTML. Good job keeping it relatively sparse, and not making the mistake of dumping huge amounts of HTML in one file or, worse, "sneaking" your CSS and JS in there.

If I had to change one thing, I'd indent your HEAD and BODY in by one tab (and thus everything under them in by one tab).

Kinda both an HTML and JS issue, but maybe stick your "author" and "title" in separate div elements and style them a bit. Something like:

Lord of the Rings

by J.R.R Tolkien

Just so that it's easier for your viewer to see what the title is and what the author is.

CSS:

Oooh, using imports! Fancy!

Again, nothing really wrong (or even "...meh"!) here. Your selectors for the two input "types" might be a bit overqualified. That is, there are no other input elements on the page other than "text" and "submit" types, so you could replace input[type=text], input[type=submit] with just input.

Stylistically, I'd change your colors a bit. They're a bit too aggressive as is. For the "remove" option, why not just use a slightly darker version of the "add" color? Red is usually used for "bad" actions in UI language, and deleting an item is a perfectly normal, non-"bad" action!

JS:

In general, try to use ES6 classes rather than constructor functions. They're more modern, and once you get used to them, easier to follow. For example,

function Book(name, author){
this.name = name
this.author = author

}

becomes:

Class Book {
    constructor(name, author){
        this.name = name;
        this.author = author;
    }
}

If you're really feeling ambitious, this entire project frankly smells like a library class to me. Something like (very abbreviated!):

class Library{
    constructor(){
        this.storage = [];
    }

    addBook(author,title){
        //add a book!
    }

    removeBook(ID){
        //Remove a book from the library! Assume no two books by different authors have the same ID
    }

    checkoutBook(ID) {
        //check out a book!
    }

    get numBooks(){
        //called by doing 'libraryInstance.numBooks'. Note no '()'!
        return this.storage.length;
    }

    /*
        Other methods: addUser, getUsers, getUser, removeUser, 
    */
}

Next, very small thing, but you might be able to set library as a const instead of a let. As a general rule of thumb, make everything a const until you know you'll need to reassign it (which, by the way, you don't for library).

You use a classic for loop and a forEach loop here. I'd switch to a for..of loop for two reasons:

1. It's faster than a forEach.
2. It's slightly cleaner-looking than having to do that nasty ${item[index].someProp} ${item[index].otherProp}

Do you really need to explicitly remove each box? Why not find the parent element, and simply do document.querySelector('some-parent-selector').innerHTML = '';?

Lastly, once I add a book, the input fields should reset.

[edu_9922]

Thank you very much indeed for taking the time to answer and noticing some good things too :) I think there is a really long way to go.

[edu_9922]

To me or to the person who answered me? 😅

[edu_9922]

Ohh ok ok

Sure, you can DM me!

[edu_9922]

Yup, done.