2
\$\begingroup\$

I'm fairly new to Javascript and am writing a piece of code to create a filtering system for a portfolio grid (a grid of work examples that filter by category when a button is selected).

While the code works just fine, I feel like the way I'm writing it is overly clunky. Can someone please let me know if there's a way I can streamline/improve this code block?

Any help would be much appreciated - thanks! :D

var gridItem = document.querySelectorAll('.grid div');
var featuredItem = document.querySelector('.featured');
var filterBtn = document.querySelectorAll('.filter-btn');


function getAttributes(event) {
  event.preventDefault();

  var thisValue = event.target.getAttribute('data-filter');

  // Get grid item filter attributes
  for (let a = 0; a < gridItem.length; a++) {
    const gridFilterAttr = gridItem[a].getAttribute('data-filterType');
    if (thisValue == gridFilterAttr) {
      gridItem[a].classList.add('active')
      gridItem[a].classList.remove('not-active')
    } else if (thisValue == 'all') {
      gridItem[a].className = 'active';
      featuredItem.className = 'featured active'


    } else {
      gridItem[a].classList.remove('active')
      gridItem[a].classList.add('not-active')
    }
  }
}

for (let b = 0; b < filterBtn.length; b++) {
  filterBtn[b].addEventListener('click', getAttributes);

}

HTML:

<!DOCTYPE html>
<html lang="en">
<head>
  <meta charset="UTF-8">
  <meta name="viewport" content="width=device-width, initial-scale=1.0">
  <title>Js Projects</title>
  <link rel="stylesheet" href="/assets/css/app.css">
</head>


  <div class="container">
    <button data-filter="all" class="filter-btn">All</button>
    <button data-filter="web" class="filter-btn">Web Project</button>
    <button data-filter="app" class="filter-btn">App</button>
    <button data-filter="ux" class="filter-btn">UX/UI</button>

    <div class="grid">
      <div class="featured" data-filterType="web">Featured Item</div>
      <div data-filterType="web">Web item</div>
      <div data-filterType="app">App item</div>
      <div data-filterType="ux">UX item</div>
      <div data-filterType="web">Web item</div>
      <div data-filterType="ux">UX item</div>
      <div data-filterType="app">App item</div>
    </div>
  </div>

  <!-- Scripts -->
  <script src="/assets/js/grid.js"></script>
</body>

</html>
\$\endgroup\$
3
  • 1
    \$\begingroup\$ It's rather difficult to judge this without the HTML, and the filter data. Can each grid item only respond to one filter button? That seems a bit weird. We basically can't test-run your code. Also, there's something to say for implementing this in pure JS, but I've gotten so used to JQuery that this stuff seems a bit verbose. \$\endgroup\$
    – KIKO Software
    Commented Apr 27, 2020 at 20:51
  • \$\begingroup\$ Hi! I've now added the HTMl for you to view :) \$\endgroup\$
    – Connor
    Commented Apr 28, 2020 at 7:28
  • \$\begingroup\$ The current question title of your question is too generic to be helpful. Please edit to the site standard, which is for the title to simply state the task accomplished by the code. Please see How do I ask a good question?. \$\endgroup\$
    – BCdotWEB
    Commented Apr 28, 2020 at 9:22

2 Answers 2

Reset to default
1
\$\begingroup\$

As mentioned in comment, it is harder to be more specific without being able to run the code, but what can I say:

  • Separate event code function and then the function, that will "do the work" itself, with all necessary parameters.
  • Extract for into separate function.

That will overall make it easier to read and cleaner + easier to test. Something like:

function getAttributes(event) {
  event.preventDefault();
  var thisValue = event.target.getAttribute('data-filter');
  setGridItems(gridItem, thisValue);
}

function setGridItems(gridItems, dataFilter) {
  for (let a = 0; a < gridItems.length; a++) {
    setGridItem(gridItems[a], dataFilter);
  }
}

function setGridItem(singleGridItem, dataFilter) {
    const gridFilterAttr = singleGridItem.getAttribute('data-filterType');
    if (dataFilter == gridFilterAttr) {
      singleGridItem.classList.add('active')
      singleGridItem.classList.remove('not-active')
    } else if (dataFilter == 'all') {
      singleGridItem.className = 'active';
      featuredItem.className = 'featured active'
    } else {
      singleGridItem.classList.remove('active')
      singleGridItem.classList.add('not-active')
    }

}

Would be cool to use foreach there, but it is a bit problematic with NodeList and support is not that broad yet, but maybe you could do that. Now when you have it a bit separated, you can continue with refactoring. I don't like that some variables are pre-cached and "global", that's why I passed gridItem variable anyway to make it local in function, but featuredItem is still global. Last function could use more of refactoring, but I'd need more info about problem for that :-)

\$\endgroup\$
4
  • \$\begingroup\$ Maybe I'm missing something because I'm not that strong on JavaScript, but can't the if (cond) else if (cond) else be simplified If (thisValue == 'all') ... else ... ? \$\endgroup\$
    – pacmaninbw ♦
    Commented Apr 27, 2020 at 22:01
  • \$\begingroup\$ NodeList.prototype.forEach is reasonably well supported. If you don't trust it, you can just spread it into an array first. [...gridItems].forEach( (if IE support is required, use Babel, as always) \$\endgroup\$
    – CertainPerformance
    Commented Apr 27, 2020 at 23:55
  • \$\begingroup\$ Hey! Thanks for your comments and answers - I have added my HTML now if that helps? \$\endgroup\$
    – Connor
    Commented Apr 28, 2020 at 7:30
  • \$\begingroup\$ Yeah I know it is reasonably supported, just didn't want to complicate it too much :-) Especially with IE missing. \$\endgroup\$
    – K.H.
    Commented Apr 28, 2020 at 8:41
1
\$\begingroup\$

In my comment I said that this would be a lot shorter if you used JQuery. Now I don't want to say that JQuery is better than plain Javascript. There may be a good reason to not use JQuery. But I do want to show what the code would look like in JQuery.

$(".filter-btn").click(function() {
  let type = $(this).data("filter");
  let filter = (type == "all") ? "*" : "[data-filterType='"+type+"']";
  $(".grid div").hide().filter(filter).show();  
});

As you can see, this is a lot less verbose. You can see a live demo here.

I think it goes too far to explain this JQuery code in detail. In broad terms it does this:

  1. Bind an event handler to the "click" JavaScript event of your buttons.
  2. Extract the filter type from the data attribute of the clicked button.
  3. Based on that it creates a filter variable that selects the grid items to be shown.
  4. First hide all grid items and then only show the filtered ones.

It is the last line that does all the work.

I intentionally didn't change the HTML so this works on the same HTML as you have now.

\$\endgroup\$
2
  • 1
    \$\begingroup\$ The code isn't really shorter because you are using jQuery. It is shorter, because you are using a different approach. You could use a very simular approach with pure JS/DOM and without jQuery and still be almost just as short. \$\endgroup\$
    – RoToRa
    Commented Apr 28, 2020 at 10:05
  • \$\begingroup\$ @RoToRa Everybody knows that jQuery has a certain size, however if you use a CDN version then it is very likely that the user has already downloaded it. I also don't have to read the jQuery code, so the code you have to read is shorter than in the question. The challenge to you is to prove your statement: Show us a version in pure JS/DOM which is almost just as short. PS: Shortness isn't everything, we should concentrate on readability. \$\endgroup\$
    – KIKO Software
    Commented Apr 28, 2020 at 14:19

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.