3
\$\begingroup\$

I have a javascript function that will accept an array of objects (from a database or websocket) and then spit out HTML <ul> lists. The code works -- but I am "self-taught" and always learning. I want to be producing professional looking/behaving code. I will be graduating from university, but job prospects in my field are low... Any insights into style or approach would be welcomed. Get the red marker out :)

The HTML output from the function is:

<ul class="prek">
  <li class="firstName">Bobby</li>
  <li class="lastName">Fisher</li>
  <li class="guardian"><span>Mommy</span><span>Fisher</span></li>
  <li class="checkIn">9:14am</li>
  <li class="phone">ewqrqr3452</li>
  <ul class="buttons">
    <li class="pageButton">Page Mommy</li>
    <li class="edit">Edit Bobby</li>
    <li class="move">Move Bobby To Group</li>
    <li class="checkOut">Check Out Bobby</li>
  </ul>
</ul>

<ul class="grade1">
  <li class="firstName">Anne</li>
  <li class="lastName">Gables</li>
  <li class="guardian"><span>Green</span><span>Gables</span></li>
  <li class="checkIn">9:14am</li>
  <li class="flag"><span class="peanuts"></span><span class="bees"></span></li>
  <li class="phone">ewqrqr3452</li>
  <ul class="buttons">
    <li class="pageButton">Page Green</li>
    <li class="edit">Edit Anne</li>
    <li class="move">Move Anne To Group</li>
    <li class="checkOut">Check Out Anne</li>
  </ul>
</ul>

The input looks like this:

children = [
    {
        group: "prek",
        firstName: "Bobby",
        lastName: "Fisher",
        guardian: {firstName: "Mommy", lastName: "Fisher"},
        checkIn: "9:14am",
        phone: "ewqrqr3452",
    },
    {
        group: "grade1",
        firstName: "Anne",
        lastName: "Gables",
        guardian: { firstName: "Green", lastName: "Gables" },
        checkIn: "9:14am",
        notes: "",
        flag: ["peanuts", "bees"],
        phone: "ewqrqr3452",
    }
];

for(const child of children) {
    addChild(child);
}

And the Javascript looks like this:

function addChild(child){

    // Create child <ul>
    const ul = document.createElement("ul");
    document.getElementById("childList").appendChild(ul);

    // Assign <ul> to a group
    if(child.group) {
        ul.classList.add(child.group);
        delete child.group;
    }
    else {
        ul.classList.add("unassigned");
    }

    // Add properties to the child as <li>
    for(let property in child) {
        // If the value is empty don't add the <li>
        if(!child[property]) {
            continue;
        }

        // Create <li> node and add class
        const li = document.createElement("li");
        li.classList.add(property);

        // Add child properties to the li
        if(typeof(child[property]) !== "object") {
            // Add simple data to the li
            li.innerText = child[property];
        } else {
            // If the data is an object go deeper put the elements into a span
            for (const nestedKey in child[property]) {
                // Don't go deeper -- it's just malformed data
                if(typeof(child[property][nestedKey]) === "object") {
                    continue;
                }

                // Create the span with data in it
                const span = document.createElement("span");
                if(parseInt(nestedKey) >= 0) {
                    span.classList.add(child[property][nestedKey]);
                } else {
                    span.innerText = child[property][nestedKey];
                }
                li.appendChild(span);
            }
        }
        ul.appendChild(li);
    }

    // CHILD BUTTONS
    const buttons = document.createElement("ul");
    buttons.classList.add("buttons");

    // Page guardian button
    const page = document.createElement("li");
    page.innerText = `Page ${child.guardian.firstName}`;
    page.classList.add("pageButton");
    buttons.appendChild(page);
    page.addEventListener("click", function(){pageGuardian()});

    // Edit child button
    const edit = document.createElement("li");
    edit.innerText = `Edit ${child.firstName}`;
    edit.classList.add("edit");
    buttons.appendChild(edit);
    edit.addEventListener("click", function(){editChild()});

    // Move child button
    const move = document.createElement("li");
    move.innerText = `Move ${child.firstName} To Group`;
    move.classList.add("move");
    buttons.appendChild(move);
    move.addEventListener("click", function(){moveChild()});

    // Checkout child button
    const checkOut = document.createElement("li");
    checkOut.innerText = `Check Out ${child.firstName}`;
    checkOut.classList.add("checkOut");
    buttons.appendChild(checkOut);
    checkOut.addEventListener("click", function(){checkOutChild()});

    ul.appendChild(buttons);
}
\$\endgroup\$
6
  • \$\begingroup\$ Welcome to Code Review! Please stop editing your post - especially the code. After getting an answer you are not allowed to change your code anymore. This is to ensure that answers do not get invalidated and have to hit a moving target. If you have changed your code you can either post it as an answer (if it would constitute a code review) or ask a new question with your changed code (linking back to this one as reference). Refer to this post for more information \$\endgroup\$ Feb 27, 2020 at 22:40
  • \$\begingroup\$ Sᴀᴍ Onᴇᴌᴀ: I apologize. I was trying to add the edits based on the answer below. When I read the stack's instuctions about bounties it said "be sure to edit your question as people leave answers"... I unchecked the answer. \$\endgroup\$ Feb 27, 2020 at 22:42
  • \$\begingroup\$ Okay I rolled it back to the edit before rotata's answer was posted. You could change the description if you feel there is something you really want to add. "I unchecked the answer." - why? \$\endgroup\$ Feb 27, 2020 at 22:46
  • \$\begingroup\$ I unchecked the answer so that I could continue to get feedback on the changes as the document is so different. \$\endgroup\$ Feb 27, 2020 at 22:47
  • \$\begingroup\$ Okay- you have other options for continued feedback - see What to do when someone answers. \$\endgroup\$ Feb 27, 2020 at 22:48

1 Answer 1

2
\$\begingroup\$

First some words about the created HTML/DOM:

In HTML it is invalid for an ul element to directly contain another ul. An ul only may contain li elements, so you should wrap the inner ul in it's own li. Unfortunately the DOM API allows you to create invalid HTML and there is no good way to notice it. And it may not become noticeable until the browser has problems rendering it.

Regarding the guardian names: You should have a space character between the span elements otherwise the browser will render it (or in case of screen readers/text-to-speech, may read it) as one one word. EDIT: Creating a space with CSS may not be recognized by a screen reader. An actual space will always be interpreted correctly. Another case were this could be a problem is copy and paste where it also will be copied without a space.

A similar accessibility problem may be the way "flag" is being rendered: Even if you use CSS to display something, empty span elements with just a class name but no content may be ignored and not read out by screen readers.


On to the JavaScript:

Instead of calling document.getElementById("childList") each time addChild is called it would be better to call it once outside and pass the reference into addChild.

In a function such as addChild which purpose is to display the child object it's unexpected for it to modify that object with delete child.group, especially since that modification isn't relevant to the further logic of the function.

At the event listener assignment it's unnecessary to wrap the listener function in an additional function. So

page.addEventListener("click", function(){pageGuardian()});

Can be written as:

page.addEventListener("click", pageGuardian);

BTW how to the listener functions determine which child they work with when called?

Finally, the code for creating the buttons are basically identical for each button so it should be extracted into a separate function.


EDIT: One more point I forgot before: You may want to look into using a template engine instead of building the HTML/DOM structure yourself.

\$\endgroup\$
5
  • \$\begingroup\$ HTML > Thank you for the <ul> catch! As for the span, I am using CSS to put a space between them. Will it still have screen reader problems? I'll move my image from CSS to the HTML. I guess that makes sense because it is content. JS > I use the delete child.group because I don't want to create another <li> element for it--I should have used continue. Thank you for that as well. pageGuardian is wrapped because I will pass the function some parameters. I just haven't coded that yet -- so it's a placeholder. I appreciate your help! I'll change the getElementById("childList") for sure! \$\endgroup\$ Feb 27, 2020 at 14:09
  • 1
    \$\begingroup\$ I really appreciated the suggestion to not delete the property from the object -- I mean, what if I used it later (I'm not planning on it... but that's some horrible code to keep straight in your mind!) \$\endgroup\$ Feb 27, 2020 at 14:21
  • \$\begingroup\$ I edited the code in the question to reflect your changes. Additionally, it caused me to rethink some of my approach. \$\endgroup\$ Feb 27, 2020 at 22:43
  • \$\begingroup\$ I've added a few more details to the missing space in the guardian's name. \$\endgroup\$
    – RoToRa
    Feb 28, 2020 at 7:56
  • \$\begingroup\$ And I've add one more point at the end. \$\endgroup\$
    – RoToRa
    Feb 28, 2020 at 8:10

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.