4
\$\begingroup\$

I wrote this page with the goal in mind of having a footer that runs along the bottom of the page that can be closed, and has a floating info box that appears when the footer is hovered over. I'm just learning JS, HTML and CSS now, so I'd appreciate any critiques on style.

Am I doing anything the long way? Is any of the JavaScript un-idiomatic? Please let me know!

What I'm not looking for advice on:

  • I know that external CSS and JS would likely be preferable. I'm using internal JS and CSS to ease learning.

This is a very broad review request. I'd like opinions on any aspect of any of the languages used.

FooterTest.html:

<!DOCTYPE html>
<html lang="en">
<head>
    <meta charset="UTF-8">
    <title>Footer Test</title>
    <style>
        body {
            background-color: lightseagreen;
        }

        #floatingBox {
            display: none;
            position: fixed;

            background-color: black;
            color: white;

            left: 0px;
            top: 0px;

            width: 50px;
            height: 50px;

            z-index: 1;
        }

        #fixedFooter {
            display: block;
            position: fixed;
            background-color: bisque;
            color: black;

            left: 0px;

            border-top: 10px double black;
            bottom: 0px;

            height: 20%;
            width: 100%;

            text-align: center;

            z-index: 0;
        }

        #closeButton {
            display: inline-block;
            position: absolute;

            right: 0px;
            top: 0px;

            color: white;
            background-color: red;

            width: 5%;
            height: auto;

            z-index: 1;
        }
    </style>
</head>
<body>
    <div id="floatingBox">
        Test Block
    </div>

    <footer id="fixedFooter">
        <button id="closeButton">X</button>
    </footer>
    <script>
        var fixedFooter = document.getElementById("fixedFooter");
        var positionText = document.createElement('p');
        var closeButton = document.getElementById("closeButton");

        fixedFooter.appendChild(positionText);

        var floatingBox = document.getElementById("floatingBox");

        var defaultDisplay = "inline-block";

        //To prevent the mouse from entering the floating box
        var posBuffer = 10;

        fixedFooter.onmousemove = function(e) {
            var x = e.pageX;
            var y = e.pageY;

            positionText.innerHTML = formatPosition(x, y) + " - "
                    + formatPosition(floatingBox.style.left, floatingBox.style.top);

            setElementPosition(floatingBox, x + posBuffer, y + posBuffer);
        };

        fixedFooter.onmouseover = function(e) {
            floatingBox.style.display = defaultDisplay;
            console.log("\tMouse In");
        };

        fixedFooter.onmouseout = function() {
            floatingBox.style.display = "none";
            //setElementPosition(floatingBox)
            console.log("\tMouse Out");
        };

        closeButton.onclick = function() {
            fixedFooter.style.display = "none";
        }

        var setElementPosition = function(element, xPos, yPos) {
            element.style.left = xPos + "px";
            element.style.top = yPos + "px";

            //console.log("Setting " + element + "'s position to " + formatPosition(xPos, yPos));
        };

        var formatPosition = function(x, y) {
            return "(" + x + ", " + y + ")";
        };

    </script>
</body>
</html>
\$\endgroup\$

1 Answer 1

Reset to default
1
\$\begingroup\$

OK here's a few pointers on the above code:

  1. IDs in css are not a good idea. The problem is that an ID can only be over-ridden by an ID, inline style or !important declaration. It's pretty well established best practice to use classes rather than IDs as a way of hooking in your css. It's great you aren't using the HTML tags themselves as selectors.

  2. It's very interesting how you've grouped your css properties. I think you've gone for logical groupings rather than the arbitrary alphabetical order that is preferred by many. This might be contentious, but I think it's better to make logical groups.

  3. JS variables are not name-spaced. This is fine in tiny examples, but will be very problematic in larger sites. For example, the very generic closeButton variable in the JS could easily be over-written somewhere else, by someone else and cause a tricky and unnecessary bug.

The same applies with css and while we're here, separating out JS hooks from class attributes used for styling will help edits to styles effecting JS functionality on the page.

Consider this as an alternative:

var FooterPopup = {
  closeButton: document.getElementById('#js-footer-popup__close-button');
}

.footer-popup__close-button {
  /* styles */
}

<div id='js-footer-popup__close-button' class='my-page__close-button'></div>

Where FooterPopup.closeButton will access the close button within the context of the object and the page. You can now port the FooterPopup anywhere else and re-use it.

Note, the js- prefix makes it very clear that the hook is used by javascript. The double underscore is nicked from BEM notation and I've used it here to define a block and it's element in a way that is clear to read and shouldn't be hit anywhere else in the site and over-ridden.

  1. Addition of hard-coded styles in the JS. You use the .style.display JS declaration to hide the element and close it. This is problematic because everywhere you want to add this functionality you'll have to directly define it. There are literally 100s of ways of closing something when you consider all the interface nuances designers might want to introduce so isolating this and making it re-usable is a good idea.

A way to do this is to use a utility class then add and remove that using JS. Then you define the closed style in css - recommended at the bottom of the file so the styles over-ride any that might be added to the element generally. Note that this has other benefits - if you wanted something to load on the page hidden you could just add this class to the element rather having to add all the properties copied from JS to the css for that element.

Here's a simple example:

.element {
  opacity: 1;
  top: 0;
  transition: all 0.2s;
}

.is-hidden {
  opacity: 0;
  top: -50px;
}

$('.element').addClass('is-hidden'); (I've used JQuery here - it's a robust, common library that takes some of the pain out of DOM manipulation, you may have considered it however and it's very informative and ultimately better, if a little harder to start with, to tackle the raw JS).

Here is the more complicated way to add and remove classes with just JS.

Finally, check out JSFiddle or CodePen. Here's a link to your code in JSFiddle: https://jsfiddle.net/ujLjr0kp/1/ - it's really useful front-end playground and very good for sharing code on Stack Exchange :-)

\$\endgroup\$
1
  • \$\begingroup\$ Awesome, thank you. I'll keep these points in mind going forwards. \$\endgroup\$
    – Carcigenicate
    Commented Dec 10, 2015 at 17:18

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.