Bulma modal prompts

[Geo-Bold]

Resolves the use of Bulma modals instead of JS prompts while retaining as much initial structure as possible. Modal creating function is easily reusable and is not rendered to the DOM until called. A garbage collection function ensures that no duplicated modals remain rendered at one time.

For the local storage reset protocol, there is similar structure to the original JS prompts and confirms. So far, there exists control over the prompt and establishment of a generic default.

Further changes would be centered around enhancing the functionality of the modal creating function to allow for more control over each modal generated as well as revisiting the current style for the prompts to include two buttons, making them larger and removing the input field when not necessary.

[anthonyisensee]

@Geo-Bold thanks so much for this! Thanks for the detailed description, as well. I'll review it and add some feedback as soon as possible!

[Geo-Bold]

The new commit resolves the conflict from @anthonyisensee prod branch.

[anthonyisensee]

Thanks for the pull request, @Geo-Bold! I can tell you put a lot of time into it, and I really appreciate it. Thank you, as well, for proactively resolving the merge conflict, as well as for your Total Modal Card Overhaul commit. That commit added a number of things I was going to request, mainly the (nearly) complete card modal with a separate modal layout for prompt and confirm!

A professor of mine once gave me some key insight that has stuck with me in all my programming since. After programming, or when reviewing others code, you have three main questions to answer:

• Does it work/run/meet the requirements? If yes, 50% of the potential value has been achieved.
• Is it readable and maintainable? - If yes, 40% of the potential value has been achieved.
• Is it well optimized? - If yes, 10% of the potential value has been achieved.

These insights guide my feedback.

Does it work?

Yes! It does, and that warrants a massive kudos to you and your work. I love how responsive the modal feels, and to the end user, that's all that matters. You've delivered the client's ask, and for that they love you! With this alone, you've achieved 50% of the goal. It is important to note this success! Anything that follows this is (in my mind, mandatory) extra credit that ensures the code is maintainable and readable.

Is it readable and maintainable?

Sort of.

First of all, the thing I love. You recognized that modal functionality was different enough from the Main.js or any other piece of the project that it warranted it's own file. I wasn't necessarily thinking of doing this, but it was totally the right decision!

That said, there are definitely some improvements that can be made!

To understand this, the idea of software orthogonality is crucial. The key idea of software orthogonality is that individuals pieces of the software can grow all on their own (or even be entirely replaced) without impacting the rest of the software. In short, that their purposes are orthogonal.

Consider this illustration of two orthogonal vectors. Because they are separated, they can grow and scale in functionality all on their own without impacting the other, or one requiring modification of the other. Additionally, because they are scaling separately, they become deeply reusable throughout the application for purposes that aren't specific to just one file or function, usable throughout the codebase without impacting other pieces of the code. And, because they aren't tied to each other, each can grow in its own direction at the fastest pace possible.

Ideal:

At the moment, however, the purposes of the two files are tied together. If one is modified, so must the other be. Code specific to Main.js is being run inside of Modal.js (here), meaning that in order to modify the functionality of Main.js one must edit Modal.js. As such, the vectors are no longer orthogonal, but are tied together. This means that they are no longer separately scalable, and that changes to one deeply impacts the other. This is not ideal!

Current state:

I think that you intuitively knew this. My evidence is that you used promises and their callbacks for the confirmation modal. While I would like to see the promises removed (as adding asynchronous programming to such a simple, locally running application with so little blocking operations needlessly increases the complexity) I love that you wanted to keep Main.js code inside of Main.js.

Secondly, let's talk object oriented (for a OO JS guide, I highly recommend reading through this example on mdn). You decided to export functions from Modal.js and import them into a Modal object in Main.js. While this does work, another way is to make Modal.js a full on separate class in its respective file and add a constructor. (For a rough template, see this code. Just note, since writing that code I've discovered that prefixing a variable with _ is no longer recommended, so you don't need to do that).

In the following, key callouts should be made to Objects, Methods, and Properties. These are three of the biggest components of proper Object Oriented programming.

// Modal.js defines a class that initializes and provides functionality to Modal objects
export class Modal() {

    // Runs once upon object instantiation/initialization. Useful for setting properties associated with the object as a whole.
    constructor(...){

        // Dynamically set a property of the class
        this.modal = document.getElementById("modal")

        ...

    }

    // Properties can also be set outside of the constructor, but you do so without using variable declaration (const, let, or var)
    example_property = "example"

    // Class methods are the things a class can do. Note that they do not require the "function" keyword
    prompt(...) {
        ...
    }

    confirm(...) {
        ...
    }

}

This functionality can then be be imported and used in other classes like so:

import { Modal } from "example/path/Modal.js";

...

const modal = new Modal()

I would request that since the rest of the repo is using OO, that you attempt to implement it with Modal.js as well. If you have any questions, definitely do some research into OO and/or feel free to ask questions. The sooner you get exposure to proper OO, the better! Just keep in mind as you build the class that it could be instantiated at the same time from other code multiple times, and that you should treat the modal state (open or closed) as an unknown.

Thirdly, there is a way to make this portion of your code significantly shorter, more readable, and modifiable. It will also replace your documentation with the code itself, which is always a massive win!

Instead of dividing the functionality up by individual elements:

function createExampleDiv(message) {

  const div = document.createElement('div')  // Layout
  div.classList.add("a", "b", "c")  // Layout
  div.setAttribute("id", "example_div")  // Layout
  div.addEventListener(...)  // Functionality

  const p = document.createElement('p')  // Layout
  p.classList.add('e')  // Layout
  p.innerHTML = message  // Layout
  p.addEventListener(...)  // Functionality

  div.appendChild(p)  // Layout

  return div

}

Divide the functionality up directly:

function createExampleDivBetter(message) {

  // Layout
  const html = `
    <div id="example_div" class="a b c">
      <p class="e">${message}</p>
    </div>
  `
  const node = new DOMParser().parseFromString(html, 'text/html').body.firstChild

  // Functionality
  node.getElementById("example_div").addEventListener(...)
  node.querySelector("p").addEventListener(...)

  return node

}

This will significantly simplify your layout life, especially after you get 20 layers of HTML deep and/or have come back to the code after you've forgotten exactly what the HTML layout looks like. For an example, see this code. Just think how miserable it would be trying to keep this all in your head and modify it in the format of the first example!

Finally, a minor tweak. To maintain consistency all code should be four space indented. Wars have been waged over 2 vs 4 space indenting, and both have advantages and disadvantages. But, as much as possible, you should do your best to maintain the style of a repository you are contributing to. If you want to make your life easier, you can set workspace specific indenting rules in VSCode (or most other IDEs) so it will handle everything for you automatically in only your one workspace.

There are other components that I may circle back around to depending on your implementation of OO, but for now these are the big things.

Is it well optimized?

It's JS. As long as there aren't any glaring optimization issues, I and most of the rest of the world could care less. ;D

Additional Specifications

I'm going to do something that most normal clients never do; give you specific coding asks for a product. My hope is that they will be helpful! :)

1. Make your code OO, with clear orthogonality between Main.js and Modal.js. Once your done, I want Modal.js to be usable in any of your future (bulma) projects by adding the Modal.js file alone.
2. Add prompt() and confirm() methods to the Modal class (please avoid modalPrompt() and modalConfirm() as such naming schemas are redundant and force the developer to type modal over and over, such as with modal.modalPrompt()).
3. Add a parameter to confirm() and prompt() for a) a generic function that runs upon success and b) a generic function that runs upon failure. These functions should be executed when the user clicks the corresponding button in the modal, and the generic functions for prompt should have the results of the input field passed in to them before execution. I believe this will be key in increasing orthogonality and will allow you to remove Promises from Modal.js while keeping Main.js functionality in Main.js. This is a bit of a complicated ask, so definitely reach out if you have questions.
4. Implement the layout/functionality separation I suggested.
5. Add the modal card footer to the modal and move any buttons to it. For prompt and confirm, please add the is-danger class to the confirm button and the is-info class to the cancel button.

Thanks again! Like I said in "Does it work?" you've done great so far. The solution you've delivered is working and end users that got a preview are happy with the feature! That's 50% of the battle, right there. That said, before the code is ready for merge and to be rolled out to all users, addressing that last 40% is important!

If you have any questions, feel free to drop a comment here or reach out via separate channels!

[Geo-Bold]

@anthonyisensee Thank you for your thorough feedback. It was instrumental in directing my efforts for this patch. I am sure the client will be more than satisfied :)

[anthonyisensee]

@Geo-Bold, in progress on my code review for this. You addressed the big issues, so I'm focusing directly on the code this time!

[anthonyisensee]

Included with your next push, add the following after this line: <p>Thank you to the following contributor: <a href="https://github.com/geo-bold">Geo Archbold</a></p>.

[anthonyisensee]

@Geo-Bold I changed my mind on where I want your contributor link added. Please do not add another p tag, but instead add Thank you to <a href="https://github.com/geo-bold">Geo Archbold for contributions</a>. to the currently existing third p tag here after the text currently contained there.

[Geo-Bold]

Included in this latest commit are the requested changes. Reach out with any questions or modifications.

[anthonyisensee]

Whoop whoop! It's live, baby! @Geo-Bold, I imagine you must feel something like this:

[Geo-Bold]

So excited to be done!