1
\$\begingroup\$

My goal was to have a single page display bulk amounts of media without the need to load new pages. I also wanted it to be able to load fast, so the generator uses a JavaScript array to display one item at a time. The user can move through the array backwards and forwards with the use of two buttons.

Images, video and audio can be added as strings in the following ways:

  • '<video controls source src="FILE.mp4" type="video/mp4"></video>',
  • '<img src="FILE.png">',
  • '<audio controls source src="FILE.mp3" type="audio/mpeg"></audio>',

const array = [
    '',
    '<p>Text\<br>Hello I am text</p>',
    '<video controls source src="FILE.mp4" type="video/mp4"></video>',
    '<img src="FILE.png">',
    '<audio controls source src="FILE.mp3" type="audio/mpeg"></audio>',

];
//variables
const arrayLength = array.length;
var For = -1;

function backForth(ev) {
    if(ev.target.className == "buttonS") {
        const output = document.querySelector('output');
        let increment = (ev.target.textContent == "Back" ? -1 : 1);
        For = (For + increment + arrayLength) % arrayLength;
        output.innerHTML = array[For];
        document.getElementById('counter').innerHTML = For + '&nbsp;/&nbsp;' + (arrayLength - 1);
}}
document.addEventListener('click', backForth);
backForth({target:{className: "buttonS"}});
<body>
    <output></output>
    <div>
        <button class="buttonS">Back</button>
        <b><span id="counter"></span></b>
        <button class="buttonS">Forth</button>
    </div>
</body>

\$\endgroup\$

2 Answers 2

0
\$\begingroup\$

The overall approach looks fine. Some suggestions:

Delegated click logic The current backForth being used both as a click listener and on the initial load feels a bit clunky since you have to pass in {target:{className: "buttonS"}}. Consider making a separate function that you pass an index to increment (either 1 or -1), and call that inside backForth, and call it with (1) initially.

Don't move buttons User interfaces are slightly friendlier when buttons stay in the same place - when they don't move based on the content size. Consider either moving the buttons above the output, or giving the output a fixed (or high minimum) height.

For variable name is really weird - it's very close to for, which is a reserved word, and regardless doesn't really indicate what the variable contains very well. Consider naming it something like displayedMediaIndex.

Variable initializers - you use var, let, and const:

  • If you're going to write in ES6+ (which you are, and you should), always use const or let to declare variables. var has a few gotchas (such as an unintuitive function scope rather than block scope, and of automatically assigning itself to properties of the global object when on the top level.) See ESLint rule no-var.
  • When deciding between const and let, use const whenever possible, since it indicates that the variable won't be reassigned, which improves readability when a reader of the code can understand that at a glance.

Prefer strict equality over sloppy equality, since sloppy equality operators have some very strange type coercion rules that a reader of the code should not have to have memorized to be entirely confident of the logic your code is implementing.

This is how I'd do it:

const medias = [
    '',
    '<p>Text\<br>Hello I am text</p>',
    '<video controls source src="FILE.mp4" type="video/mp4"></video>',
    '<img src="FILE.png">',
    '<audio controls source src="FILE.mp3" type="audio/mpeg"></audio>',
];
const mediasLength = medias.length;
let displayedMediaIndex = -1;
const output = document.querySelector('output');
const paginator = document.querySelector('.paginator');
const changeMedia = (indexChange) => {
    displayedMediaIndex = (displayedMediaIndex + indexChange + mediasLength) % mediasLength;
    output.innerHTML = medias[displayedMediaIndex];
    paginator.textContent = displayedMediaIndex + ' / ' + (mediasLength - 1);
};
document.addEventListener('click', (event) => {
    if (event.target.className == 'buttonS') {
        changeMedia(event.target.textContent === "Back" ? -1 : 1);
    }
});
changeMedia(1);
<div>
    <button class="buttonS">Back</button>
    <b><span class="paginator"></span></b>
    <button class="buttonS">Forth</button>
</div>
<output></output>

\$\endgroup\$
1
  • \$\begingroup\$ Your snippet is incomplete, "ReferenceError: ev is not defined" \$\endgroup\$
    – user231248
    Commented Jan 1, 2021 at 18:43
0
\$\begingroup\$

Code and style

  • Odd name to use as a variable For you have been forced to capitalize it which is not in line with JS naming conventions. ES6 lets you use keywords as property names, but I would not encourage using them even if you use capitals to avoid syntax errors. Maybe next would be more appropriate.

  • I have not seen collapsed closing blocks in a long time. I was once into them as they could save considerable space. However in the long run they make code much harder to read, and worse, modifying code in collapsed closing blocks becomes a guessing game to the point of defining them pure evil.

  • I would normally argue against inserting HTML via markup (setting innerHTML) but in this case to use the DOM would add needless complexity where not needed.

  • For counter.innerHTML use counter.textContent and you don't have to use the ' / '

  • Comments (lack or whim) are what some programming teachers used as an excuses to mark down/up assignments. Comments are generally useless, and at worse dangerous. The comment // variables comes under the heading useless.

  • Query the DOM once rather than many times inside code.

  • Use strict equality ===, its faster (not that it matters in this case). Using strict equality is just a good habit when coding JS, a real pain if you are switching between C like languages. In this case there is no ambiguity so this is just to let you know that JS has two types of equality and inequality ==, != and ===, !==.

  • Avoid code noise, (ev.target.textContent == "Back" ? -1 : 1); does not need the ()

  • Spaces after if. eg if( becomes if ( same with for, while, else, etc.

  • Adding data to the element's dataset can simplify some of the work of determining an action for a click. See rewrite

  • Break up the function "backForth" into two roles "event listener" and "media navigator" See rewrite.

  • Protect your codes state, wrap it in a function. See rewrite.

UI

  • There is nothing worse than a button that is intended to be clicked repeatedly that moves and needs to be hunted down to click again. Fix its position or move it to some location that does not make it move when the content changes.

  • Consider using the button elements title to provide additional information.

Rewrite

Rewrite addresses most of the points above and adds some. It is as an example only.

There is no right or wrong when coding, you will develop a style as you learn and gain experience.

You will likely tend towards the language's conventions or even discover a better style.

;(()=>{
    const query = (qStr, el = document) => el.querySelector(qStr);
    const content = (HTML, title) => ({HTML, title});
    const options = [
        content('', "Empty selection"),
        content('<p>Text\<br>Hello I am text</p>', 'Some text'),
        content('<video controls source src="FILE.mp4" type="video/mp4"></video>', 'Video FILE.mp4'),
        content('<img src="FILE.png">', 'Image FILE.png'),
        content('<audio controls source src="FILE.mp3" type="audio/mpeg"></audio>', 'Audio file.mp3'),
    ];
   
    document.addEventListener("click", clickEvent);

    const output = query("output");
    const counter =  query("#counter");
    const back =  query("#backBtn");
    const forward =  query("#forwardBtn");
    const length = options.length;
    var current = 0;
    backForth(0);

    function backForth(step) {
        current = (current + step + length) % length;
        output.innerHTML = options[current].HTML;
        counter.textContent = current + ' / ' + (options.length - 1);
        back.title = "Prev " + options[(current + length - 1) % length].title;
        forward.title = "Next " + options[(current + 1) % length].title;
    }

    function clickEvent(e)  {
        if (e.target.className === "buttonS") { backForth(Number(e.target.dataset.step)) }
    }  
})();
<button class="buttonS" id="backBtn" data-step="-1">Back</button>
<b><span id="counter"></span></b>
<button class="buttonS" id="forwardBtn"  data-step="1">Forth</button><br>
<output></output>

\$\endgroup\$

Your Answer

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