Refactor addAuthor (JavaScript) for clarity and maintainability

[Janell-Huyck]

Fixes #294

Refactor of addAuthor JavaScript Function

Overview

The addAuthor function has been refactored to improve readability and maintainability and to follow best practices in JavaScript and DOM manipulation. The refactoring addresses issues related to global state manipulation, inline styles, and event handlers, improving the overall structure of the function.

Changes

Isolated Global State Manipulation:

Removed the direct manipulation of the count variable within the function, ensuring a more predictable and isolated behavior.

Use of Template Literals:

Switched to using template literals for HTML string construction, enhancing readability and maintainability.

Removed Inline Styles and Event Handlers:

Inline onclick handlers and styles have been removed from HTML string construction.
Event listeners are now added programmatically in JavaScript, improving separation of concerns.

Function Decomposition:

Broke down the original addAuthor function into smaller, single-responsibility functions such as createAuthorElement, createInput, createDeleteButton, and updateAddAuthorButton.
This decomposition aids in understanding each part of the function and makes it easier to maintain and test.

Improved Variable Naming:

Adopted more descriptive variable names to clearly convey the purpose and usage of each variable.

Element Creation and Attribute Assignment:

Created a utility function createElementWithAttributes to streamline the process of element creation and setting attributes, reducing redundancy and improving code clarity.

Impact

These changes lead to a codebase that is easier to understand, modify, and test. It aligns with modern JavaScript practices and improves the scalability of the code.

New tests

Added a new feature test to check the functionality of add, edit, and delete authors within both the creation and the edit page for other_publication. All publication types inherit their CRUD functionality from PublicationsController, so testing one is the same as testing all.

[Janell-Huyck]

Tests are passing when the initializer is removed - they should fail. - Check with a feature test

[Janell-Huyck]

Tests are passing when the initializer is removed - they should fail. - Check with a feature test

This isn't correct. What's going on is the files are saved/cached somehow, so when we run the test after taking out the initializer (or even commenting out all the functions in add_delete_authors.js), we end up using saved functions.

To fix this, from your terminal run rails assets:precompile after disabling the loading. Then the tests will fail. You will need to run it again after re-enabling the tests in order for things to work properly.

NOTE: there is a bug in our setup that creates a blank application.js file inside the folder app/assets/builds when we run a precompile. You MUST delete this file, otherwise you end up disabling the ability of the javascript to run on your local instances.

[hortongn]

@Janell-Huyck Bug: after adding four total authors, when I click the remove button for the fourth author it removes the third author instead.

[Janell-Huyck]

@Janell-Huyck Bug: after adding four total authors, when I click the remove button for the fourth author it removes the third author instead.

It looks like it removes a specific author location regardless of how many authors are added: Once it removes the third if I have 4,5,6,7... and another time it always removed the second author. 😕

[Janell-Huyck]

Added "WIP" because testing was not catching the issue discovered by Glen, and expanded testing is failing.

[hortongn]

@Janell-Huyck I did some extensive testing and the adding/deleting works fine. The correct authors get deleted and it saves fine. However, 2 issues:

1. I can never remove the first author, right? If I've entered five authors and want to eventually remove the first, there's no remove button for them. I wouldn't block the PR for it, but maybe it should be a new issue?

2. The remove button doesn't work for me the first time I land on the new publication page. If I'm on /publications and click new book, the remove button on /books/new does nothing until I manually reload the browser page. Same happens if I'm on /publications and click the edit link for an existing publication. The remove buttons don't work until I reload that edit page. This is typically a turbolinks problem. Turbolinks fast loads the page from cache and doesn't catch the JS until the page is manually reloaded/rendered. There are ways to fix this with a turbolinks:load event.

[Janell-Huyck]

@Janell-Huyck I did some extensive testing and the adding/deleting works fine. The correct authors get deleted and it saves fine. However, 2 issues:

1. I can never remove the first author, right? If I've entered five authors and want to eventually remove the first, there's no remove button for them. I wouldn't block the PR for it, but maybe it should be a new issue?

I agree this should be a new issue. There was not an option to remove the first author before the rewrite. I agree that you SHOULD be able to, but that is a new functionality for this app.

2. The remove button doesn't work for me the first time I land on the new publication page. If I'm on /publications and click new book, the remove button on /books/new does nothing until I manually reload the browser page. Same happens if I'm on /publications and click the edit link for an existing publication. The remove buttons don't work until I reload that edit page. This is typically a turbolinks problem. Turbolinks fast loads the page from cache and doesn't catch the JS until the page is manually reloaded/rendered. There are ways to fix this with a turbolinks:load event.

Thanks for the tip - I'll check this out.

[Janell-Huyck]

Changing:
document.addEventListener('DOMContentLoaded', function() {
to
document.addEventListener("turbolinks:load", function() {
seems to have fixed the Remove button not working on the initial page-load.

[hortongn]

@Janell-Huyck buttons work great now. I'm going to squash commits on merge since there are so many of them.