0

I am working on a vanilla JS / jQuery application that grabs data from session storage and displays it in the browser. I have created a "Box" class, which represents a box (i.e., html card) on the browser page. The constructor has various properties like name: this.name and so forth.

There are two different types of boxes -- Group and Subject -- so I created two subclasses class GroupBox extends Box and class SubjectBox extends Box.

Here is the way it is designed to work: on the initial page load, we call a function displayGroupBoxes to populate the new GroupBoxes. That function loops through a bunch of data in session storage and creates a new GroupBox() on each iteration of the loop. Back up in the class constructor, for each time a new GroupBox is instantiated, we create HTML tags to display the data and append them to the page. That DOM manipulation code is located inside a GroupBox class method that gets called inside the class constructor.

One more thing, each GroupBox has associated "subjects", which is why we have the SubjectBox class. When a user clicks on a GroupBox, we find the associated SubjectBox objects and display them on the page.

Ok, so everything is working here, except I've run into one problem. The displayGroupBoxes() function grabs the associated subjects and passes them back up to the constructor to create HTML tags for the data and append it to the page. I got this working when all the DOM manipulation code was inside the constructor. However I received a code review from a colleague and they said it would be better to separate out the DOM manipulation code into a class method for readability, i.e. so it wasn't all inside the constructor. Once I did this, I was no longer able to access the data being passed up from the displayGroupBoxes function.

I know there is a lot of very project-specific code here, but I was hoping with that explanation, that someone might be able to look at the code and understand why I can't access the properties in the displayGroupBoxes function from the GroupBox class method createNewElements.

//////////////////////////////////////////////////////////////////////////////
//  CLASSES \\
//////////////////////////////////////////////////////////////////////////////

/////////////////////////
// BOX Parent Class
/////////////////////////

/**
 * Represents a box.
 * @constructor
 * @param {object} boxOptions - An object to hold our list of constructor args
 * @param {string} name - Name associated with account
 * @param {string} type - Type associated with account
 */
class Box {
  constructor(boxOptions) {
    this.name = boxOptions.name;
    this.type = boxOptions.type;
    this.logo = boxOptions.logo;
  }
}

/////////////////////////////
// SubjectBox SubClass
////////////////////////////

/**
 * Represents a SubjectBox. These are subjects, like individual people, associated with a given group. We don't see these on initial page load. They get created (or if already created, toggled to hidden/visible) when a user clicks on a group.
 */

class SubjectBox extends Box {
  constructor(subjectOptions) {
    super({
      name: subjectOptions.name,
      type: subjectOptions.type,
      logo: subjectOptions.logo,
    });
    this.name = subjectOptions.name;
    this.type = subjectOptions.type;
    this.logo = subjectOptions.logo;
    this.subjectId = subjectOptions.subjectId;
    this.container = document.createElement("div");

// ---> a bunch of code to create HTML tags and append to page
}

/////////////////////////
// GroupBox SubClass
/////////////////////////

/**
 * Represents a GroupBox. New group boxes are instantiated when the "displayGroupBoxes()" function is called. This is what we see on initial page load.
 * @constructor
 * @param {object} groupOptions - An object to store our list of constructor args
 * @param {array} subjectBoxes - An array to store subject boxes so that once they've been instantiated, we can store them here and then toggle to hidden/visible.
 */

class GroupBox extends Box {
  constructor(groupOptions) {
    super({
      name: groupOptions.name,
      type: groupOptions.type,
      logo: groupOptions.logo,
    });

// Use this array to store the subject boxes that have already been created, so if the user clicks on the same subject, we can grab those boxes and show them rather than rebuilding.

    this.subjectBoxes = [];
    this.name = groupOptions.name;
    this.type = groupOptions.type;
    this.logo = groupOptions.logo;

    // Create HTML Elements and Append To Page

    this.createGroupBox();
  }

  // Class method to create HTML tags and append to page

  createGroupBox() {
    // Create container div for box
    const newGroupBox = document.createElement("div");
    newGroupBox.className = "group-box-container";

    // ----> A bunch of code here to create name and title, append to page. Now we need to display image icons of the subjects that are associated with the group:

    // Container for SubjectBox Icons
    const mainContainer = document.createElement("div");
    mainContainer.className = "box-main-container";

    // Icons
    const iconsContainer = document.createElement("div");
    iconsContainer.className = "box-icons-container";

    // Loop through Icons object, which contains arrays of subjects, to create an img tag for each one and append to iconsContainer.

   //// ------ >> this.subjectIcons is now returning undefined << \\


    let icons = this.subjectIcons;
    for (let i in icons) {
        let iconDiv = document.createElement("div");
        iconDiv.className = "box-icon-div";
        let icon = document.createElement("img");
        icon.src = icons[i];
        icon.className = "box-icon";
        iconDiv.append(icon);
        iconsContainer.append(iconDiv);
        mainContainer.append(iconsContainer);
        i++;
    }

    newGroupBox.append(mainContainer);

/////////////////////////////////////////////////////////////////
    // EVENT LISTENER. -- when the user clicks on a GroupBox, it runs a function to show us the subjects associated with that group
////////////////////////////////////////////////////////////////

    newGroupBox.addEventListener("click", () => {
      /// --> some code to hide the GroupBoxes that are already on the page so we can display SubjectBoxes instead

      // build subject boxes
      const boxesContainer = document.getElementById("boxes-container");

      // Check to see if subject boxes have already been created. If not, instantiate the new subjectBox objects. Otherwise, if they have already been built, loop through and set them to visible

      if (!this.subjectBoxes.length) {

// If subject boxes haven't been created yet, meaning if the user hasn't clicked on this group yet, map over the array of subjects passed up from the `displayGroupBoxes()` function and create a `new SubjectBox` object for each item

     //! -----> groupOptions.subjects is returning undefined << \\

        this.subjectBoxes = groupOptions.subjects.map((subject, i) => {
          return new SubjectBox({
            name: groupOptions.subjects[i].entry_text,
            type: groupOptions.subjects[i].entry_type,
            logo: groupOptions.subjects[i].icon_link,

            subjectId: groupOptions.subjects[i].subject_id,

            // Set the container for HTML
            container: boxesContainer,
          });
        });

// Otherwise if subject boxes have already been created previously, toggle them to visible
      } else {
        for (const box of this.subjectBoxes) {
          box.show();
        }
      }

    // Append group box to document inside the same container we created for GroupBoxes:

// -------> groupOptions.container is returning undefined ----- \\

    groupOptions.container.append(newGroupBox);
  }
}


Here is the displayGroupBoxes() function that is referenced in the above code block:

// --------- Populate Group Boxes --------- \\

function displayGroupBoxes() {
  // Initialize array to hold collection of group boxes
  let groupBoxes = [];

  // Grab the "boxes-container" div from HTML and set it to variable "boxesContainer"
  const boxesContainer = document.getElementById("boxes-container");

  // Loop through the data to grab all the "B" type group boxes
  // Create a new GroupBox object for each item in the array

  
  for (let arr in data) {
    if (data[arr].entry_type === "B") {
      // Find all "Subject" ("P") boxes associated with item and store in variable "subjectGroup". Then grab all the icon image URL's
      
      let subjectGroup = data.filter(
        (group) =>
          group.group_number === data[arr].goto_group &&
          group.entry_type === "P"
      );

      let subjectGroupIcons = [];
      for (let i in subjectGroup) {
        if (
          subjectGroup[i].group_number === data[arr].goto_group &&
          subjectGroup[i].entry_type === "P"
        ) {
          subjectGroupIcons.push(subjectGroup[i].icon_link);
        }
      }


      // Create new GroupBox objects
      let groupBox = new GroupBox({
        name: data[arr].entry_text,
        type: data[arr].entry_type,
        logo: data[arr].icon_link,
       

        //! I'm not able to access these from the new "createGroupBoxes" function...

        // Set the container for HTML
        container: boxesContainer,
        // Pass subjects to GroupBox
        subjects: subjectGroup,
        // Pass subject icons up to GroupBox
        subjectIcons: subjectGroupIcons,
        // Pass headers (e.g. "Tennis Group") up to GroupBox
        headers: headerGroup,
      });

      // Push each groupBox object into the groupBoxes array.
      // This is just for convenience so we can see all the group boxes easily if we need to.
      groupBoxes.push(groupBox);
    }
  }
}

There is a lot here but hopefully it is clear what I am trying to do. I want to pass the container, subjects, and subjectIcons properties up from this displayGroupBoxes function to the class method that creates HTML tags for the data and appends it to the page. This worked when I had all the DOM manipulation code inside the class constructor, but when I moved it outside the constructor to it's own class method for readability, everything worked except for these properties being passed up. I must be calling them wrong but I don't understand the issue.

Mickey Vershbow
  • 215
  • 3
  • 17
  • 1
    "*I received a code review from a colleague and they said it would be better to separate out the DOM manipulation code into a class method for readability, i.e. so it wasn't all inside the constructor.*" - not just that, but it also shouldn't be **called** from inside the constructor. The constructor should only concern itself with setting up the instance properties. That way, your subclass can finish initialisation before the method is called. – Bergi Nov 27 '21 at 17:11
  • 1
    "*my code editor added the parentheses here, I don't understand why*" - probably because you used commas instead of semicolons there. – Bergi Nov 27 '21 at 17:14
  • Very helpful comments, understood and I will work on this, thank you! – Mickey Vershbow Nov 27 '21 at 17:16
  • "*`for (let i in icons)`*" - [don't use `for…in` enumerations on arrays](https://stackoverflow.com/q/500504/1048572)! – Bergi Nov 27 '21 at 17:20
  • @Bergi Thank you yes this is a weird one! My colleague made this comment as well. It looks like I'm using the wrong type of loop since the data seems like it should be an array, but actually what I'm getting back from the data is an object with multiple arrays! – Mickey Vershbow Nov 27 '21 at 17:21
  • 1
    @MickeyVershbow to follow up on Bergi's comment, you should use [for..of](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/for...of) instead. If there's any chance you can paste complete code for these classes, I can offer a thorough review later today or tomorrow – Mulan Nov 27 '21 at 17:34
  • 1
    @Mulan ok yeah I see your point thank you, I will do this! – Mickey Vershbow Nov 27 '21 at 17:36

2 Answers2

0

The issue is in the GroupBox constructor it is only defining name, type, and logo. You will need to add in your additional properties in the class definition for them to be available.

  constructor(groupOptions) {
    super({
      name: groupOptions.name,
      type: groupOptions.type,
      logo: groupOptions.logo,
      container: groupOptions.container,
      subjects: groupOptions.subjects,
      subjectIcons: groupOptions.subjectIcons || [],
      headers: groupOptions.headers
    });
GenericUser
  • 3,003
  • 1
  • 11
  • 17
  • Wow ok cool thank you!! I will try this now. – Mickey Vershbow Nov 27 '21 at 16:19
  • 1
    This worked! I also had to add it to the constructor as `this.container = groupOptions.container` etc. Thank you so, so much! – Mickey Vershbow Nov 27 '21 at 16:32
  • 1
    Sure thing. I just made an edit to clarify the `groupOptions` aspect. Those values I provided should be considered the defaults. – GenericUser Nov 27 '21 at 16:33
  • Oh, interesting, I just tried that and it broke things, so I must be misunderstanding. I just edited my question to reflect my implementation, based on my original understanding of your answer, can you tell me if that's correct? It's working, but I want to make sure I'm doing things correctly. If not, how can I set the default values as you suggested in your recent edit? – Mickey Vershbow Nov 27 '21 at 16:41
  • 1
    You can go a few ways with this. One way is to just set the defaults in the class constructor itself `subjectIcons: groupOptions.subjectIcons || []`. However this can also be defined in your implementation where you are instantiating the `GroupBox` instances and passing values to the constructor. In this case that looks like the `displayGroupBoxes` function. I forget that JavaScript lacks interfaces so you might just want to hardcode the defaults in the constructor. – GenericUser Nov 27 '21 at 16:53
  • 1
    Updated answer to reflect these comments. – GenericUser Nov 27 '21 at 16:58
  • 1
    This is wrong. The `GroupBox`-specific properties should **not** be passed to the `super()` call. The parent constructor should not define a `container`, that's something only group boxes have. – Bergi Nov 27 '21 at 17:13
  • @Bergi I was wondering about this as well. I just removed the `GroupBox`-specific properties from the `super()` call so they are only defined in the `GroupBox` class constructor. – Mickey Vershbow Nov 27 '21 at 17:18
  • @Bergi @GenericUser I just updated my question to reflect my last comment. I'm defining those properties from `displayGroupBoxes()` function in the `GroupBox` class constructor now and everything works. – Mickey Vershbow Nov 27 '21 at 17:20
  • 2
    @MickeyVershbow Please [put that in an answer](https://stackoverflow.com/help/self-answer) rather than editing your question to no longer show the code you asked about – Bergi Nov 27 '21 at 17:21
0

Sharing the answer here based on the responses I got. The answer I marked as correct is almost right, except the properties shouldn't be passed in the super() call. I needed to define the properties in the class constructor in order to be able to pass data to them from the displayGroupFunctions later on.

Now as @Bergi mentioned, I still need to call the class method createGroupBox from outside the constructor, but the following code addresses my original question:

class GroupBox extends Box {
  // PARAM: groupOptions = {}
  constructor(groupOptions) {
    super({
      name: groupOptions.name,
      type: groupOptions.type,
      logo: groupOptions.logo,
    });
    // Use this array to store the subject boxes that have already been created, so if the user clicks on the same subject, we can grab those boxes and show them rather than rebuilding.
    this.subjectBoxes = [];
    this.name = groupOptions.name;
    this.type = groupOptions.type;
    this.logo = groupOptions.logo;

 -----> new code:
    this.container = groupOptions.container;
    this.subjects = groupOptions.subjects;
    this.subjectIcons = groupOptions.subjectIcons;
    this.headers = groupOptions.headers;

----> this should be called from outside the constructor 

    // Create HTML Elements and Append To Page
    this.createGroupBox();
  }
Mickey Vershbow
  • 215
  • 3
  • 17
  • 1
    abstracting with classes and subclasses can be a very difficult thing to get right. often times it ends up feeling more cumbersome than effective. feel free to include the *complete* classes and i'll try to give you some pointers – Mulan Nov 27 '21 at 17:38
  • That would be incredible. I'm away from my desk for the next several hours so I'll do this tonight. Thank you for the help, I'm learning a ton. – Mickey Vershbow Nov 27 '21 at 19:30
  • @Mulan I am working on some improvements I know I need to make before posting the rest of the code. Can you help me understand where I should call `this.createGroupBox()`, if not in the constructor? I want the function to run every time a new GroupBox object is instantiated. – Mickey Vershbow Nov 29 '21 at 02:51
  • 1
    it's difficult for me to advise without seeing the entire picture. Whenever you post, feel free to comment again and I'll take a look – Mulan Nov 29 '21 at 17:21