Without seeing the HTML and CSS for the page I'm going to be making assumptions about the effects that the JavaScript has. Feel free to include the other files if the lack of context leads me to incorrect conclusions.

const quoteText = document.getElementById("quote-text");
const quoteAuthor = document.getElementById("quote-author");
const quoteCitation = document.getElementById("quote-citation");

I like the use of const here, and throughout the file. I find the code much easier to reason about when I don't have to wonder whether I might have overlooked a change in value of a variable.

Warning: const only prevents overwriting a variable. It does not make objects immutable.

Example

const a = 3;
a = 4    // TypeError: invalid assignment to const 'a'

const b = {fruit: "apple"}
b = {fruit: "pear"}    // TypeError: invalid assignment to const 'b'
b.fruit = "pear"    // No error: b == {fruit: "pear"}

export async function fetchQuotes() {
    const response = await fetch('data/quotes.json');
    if (!response.ok) {
        throw new Error(`Server returned ${response.status} ${response.statusText}`);
    }
    const data = await response.json();
    if (!Array.isArray(data.quotes)) {
        throw new Error('Invalid data structure: missing quotes array');
    }
    return data.quotes;
}

Without seeing the HTML, I can't tell what will be displayed to a user on a slow connection while they wait for this function to return, so maybe you have already addressed this. If not, the developer window of most browsers will have an option to simulate a slow connection, making the function take several seconds to return. This will allow you to see what the page looks like during loading the quotes and decide whether there's anything you'd like to be different.

export function getRandomQuote(quotes) {
    const randomIndex = Math.floor(Math.random() * quotes.length);
    return quotes[randomIndex];
}

The previous 2 functions are prefixed export. Is this because they also need to be called from another module? If not, the redundant use of export won't break anything, but it may make future readers of the code wonder if there is more complexity that they've missed.

function displayQuote(quoteObj) {
    quoteText.textContent = quoteObj.quote;
    quoteAuthor.textContent = quoteObj.author;
    quoteCitation.textContent = quoteObj.citation;
}

function scaleText() {
    const containerWidth = document.querySelector(".quote-container").clientWidth;
    const textWidth = quoteText.clientWidth;
    const scaleFactor = containerWidth / textWidth;

    if (scaleFactor < 1) {
        quoteText.style.transform = `scale(${scaleFactor})`;
    } else {
        quoteText.style.transform = "scale(1)";
    }
}

Rather than repeat quoteText.style.transform = in both the if block and the else block, I would probably use the minimum of the scaleFactor and 1 (which will have exactly the same effect). That's just my personal preference - which style a particular person finds more readable will depend on what they are used to.

So instead of

    if (scaleFactor < 1) {
        quoteText.style.transform = `scale(${scaleFactor})`;
    } else {
        quoteText.style.transform = "scale(1)";
    }
}

I might write

    quoteText.style.transform = `scale(${Math.min(scaleFactor, 1)})`;

Is it your intention to reduce the text size rather than wrapping the text onto the next line? It's worth narrowing the window as far as it will go to see how small the text becomes, and whether it is still readable.

If you decide to instead keep the text size constant and change the width of the text container, you can use .style.width instead of .style.transform. In this case you may also want to increase the height to allow for the text wrapping onto more lines.

Whichever approach you choose (shrinking the text or wrapping it onto the next line), it's worth considering doing this with CSS instead of JavaScript. This will allow the browser to handle adjusting the size automatically, rather than you having to listen for resize events from JavaScript. Although JavaScript now runs much faster than in the past, you may still find that using CSS results in smoother changes as you resize the window, particularly for anyone accessing the site from a slower machine (but test both rather than trusting my assumption).

You may even find the code is simpler in CSS as it is designed for this kind of purpose.

For example, to make an element usually 100 pixels wide but never more than 80% of the viewport width, you could set both a width and a max-width:

#element-id {
    width: 100px;
    max-width: 80vw;
}

async function populateQuote() {
    try {
        const quotes = await fetchQuotes();
        const randomQuote = getRandomQuote(quotes);
        displayQuote(randomQuote);
        requestAnimationFrame(scaleText);
        window.addEventListener("resize", scaleText);
    } catch (error) {
        console.error(`Error loading quotes: ${error.message}`);
    }
}

The console.error will be useful for a technical user who knows where to look, but non-technical users may be left wondering whether the page is broken or just slow. Without access to the HTML I don't know what the page looks like when the loading fails. It's worth considering updating the quoteText field with a message about the failure for non-technical users, in addition to the full error message in the console.

You could also send the whole error to the console instead of just the message:

        console.error(error);

instead of

        console.error(`Error loading quotes: ${error.message}`);

This will display in the console as an object that can be expanded to show all its properties, rather than just the message property. In the simple case where message is the only property this is no advantage, but I mention it in case you later decide to add more complex error types.

// Load the initial quote
populateQuote();

// Listen for click events on the "New Quote" button after DOM has loaded

On a small page the gap between the button first being visible and the event listener being added is likely to be too small to matter, but in general you can avoid any user confusion by making the button default to disabled in the HTML, and only enable it after the event listener has been added. In addition to avoiding a confusingly unresponsive button during a slow load, this also means that the button will be disabled for people who have JavaScript disabled, or if an error prevents your JavaScript from running. You could also include a message in the HTML that states that JavaScript is required for this page, and then use JavaScript to remove the message so it only remains visible for users that have JavaScript disabled.

document.addEventListener('DOMContentLoaded', () => {
    document.querySelector('.btn').addEventListener('click', populateQuote);
})

I assume at this point the page is fairly simple and only has one button with class btn, so this will work fine. However, if at any point in future you (or anyone else) adds another button somewhere on the page and happens to give it the class btn, querySelector will only apply this event listener to one of them, and it may not happen to be the one you want. I would be inclined to use an id rather than a class so you don't have to think about this for any future changes.

If you prefer to use a class (perhaps because you want the option of having more than one button that gets a new quote), I'd recommend a more semantic class name such as new-quote-button. You would also need to use querySelectorAll to return all of the elements with that class rather than just one, and then iterate over them to add the event listener to each one.

Paying for static sites

Separately from the review, since you mentioned wanting to avoid paying for static sites:

The only reasons I can think that you might need to pay for static sites are:

• You want your own domain name
• You have a huge amount of traffic (many users or very frequent users)
• You have a very large site

If those don't happen to apply to you, there's no reason you'd need to be limited to just 3 free sites. There are companies that will host your static sites free of charge, provided you don't mind the URL being more like yourname.hostcompany.com rather than yourname.com. I'm not looking to advertise any of them but a quick search will throw up a long list.

posted over 1 year ago

CC BY-NC-SA 4.0

1y ago

trichoplax‭

561 reputation 0 15 56 20