Better way to write this pice of jQuery (toggles, addclasses, etc.)

[Dennis]

I’m quite the amateur when it comes to Javscript and jQuery but hey… it works like a charm, but there must be a better way to write it. Seems awfully redundant.

CodePen

[Paulie_D]

I’m no JS/Jquery guru but what exactly is that supposed to do…’cause it doesn’t seem to do anything now.

Are you just adding and removing classes?

…and why aren’t there any classes in your CSS.

[Dennis]

I’ve updated the codepen. I haven’t added any CSS since I don’t think it’s that important in this case. The code itself is working just as I want, I just think there should be a better way of writing it since it seems quite redundant in some ways.

What I want it do to is basically this:

Click either of the buttons, if there is a .activated on the other ones, remove them, add .activated to the button that have been clicked. Depending on which button that is clicked a specific class will be added to the ul, while the other two classes should be removed if they are there. In between removing and adding the classes a small delay and some fadein/out transitions take place. This is not really needed, it just looks good.

[Paulie_D]

As I said, I’m no guru, I basically copy & paste from the API docs adjusting as I go to see what happens.

A couple of things I have learned…if you are targeting a one of a group of elements give them something in common

You are using individual classes

<button class="one-trigger">One</button>
<button class="three-trigger">Three</button>
<button class="four-trigger">Four</button>

Switch this to, perhaps

<button class="trigger one">One</button>
<button class="trigger two">Three</button>
<button class="trigger four">Four</button>

and you can target them all with

$('button.trigger').on('click')

Following on..

You are using e.preventDefault(); but a button element has no default so you can probably lose that.

My reason for asking for CSS classes was to see what actually happens. If you are adding an ‘activated’ class to a particular button, it’s nice to have a visual representation to double check it’s actually working.

See this for instance: http://codepen.io/Paulie-D/pen/CkscE

Now I admit…all it’s doing is adding the active class to the clicked button and nothing more…so far….but we (and the rest of the CSS-Tricks community) can build on that.

Seriously, I just read the API docs, and examples and practised. It’s a lot of fun :)

[Josh]

There’s always a better write to write JavaScript, I think. I’m constantly going back and rewriting my old JS as I learn more about the language.

Your code IS redundant, and most of it is unnecessary. It’s probably exactly how I would have written it a year ago. This is how I would write it today: http://codepen.io/JoshBlackwood/pen/Hwrzh

Please forgive the liberties taken with styling and the extra classes on the buttons, I just needed more visual cue as to what was happening, so I imported Bootstrap 3 for some decent defaults.

Major changes:

• Changed specific trigger classes per button to a generic .trigger class. They weren’t needed, and it’s more flexible this way.
• Added a data-number="n" attribute to each button. This is where the JS gets the correct number from now. Keeps your classes clean, stores the data where data belongs, is easily accessible from the script.
• Changed .n-products-view to n-product-view in the interest of consistency, and not needing to script a way to change the plurality of the class on the fly.
• Reduced JS to one block, using $(this) to grab the data-number for each button and assemble a class name for the ul based on that info.

It’s still not as clean as I’d like. If you didn’t need that clearfix class it’d be easier, as my current code is clearing and resetting the clearfix class every time it runs.

Cheers!

[Paulie_D]

See i knew there would be an expert along….

So if we knew how the menu was to be affected we could help more.

Now I have to dig through @Josh ‘s code so I can comment it to understand what is happening at each stage and use it later.

:)

[Josh]

Haha, thanks @Paulie_D, but I’m no expert. Still learning a little at a time, like many of us here. You’d already implemented a few of the changes I did . . . I didn’t see your post until after I’d posted.

I also went back and commented my code a bit, to save you the trouble. Should have done that to start!

@Dennis, I’m just sort of guessing here, but would I be correct in assuming that you’ll be showing only the specified number of list items given in the class when a button is clicked? If so, I can help you do that fairly cleanly.

[Dennis]

I’ve updated to CodePen somewhat hastly as I am not at home right now, will come back with a “better” update. But you should be able to see what’s actually supposed to happen.

@Paulie_D Yeah, I know what you mean. I normally just write it all out to begin with to get it working, so that I can then begin to optimize. I find that I learn better that way.

@Josh Yes, that’s basically what I do. Depending on what the user choose, the list shows, one, three, or four items per line so to speak. EDIT: Regarding the clearfix, it’s not needed, I haven’t cleaned everything up just yet. ;)

[Josh]

Ahhhh, okay. After viewing your updated Pen, I see what you want. I’ve updated my Pen to reflect this, just for completeness’ sake.

I haven’t added any of the animation stuff to my Pen, mostly because I’m not very good at animation timing and such.

http://codepen.io/JoshBlackwood/pen/Hwrzh

[Dennis]

@Josh That’s so cool! Massive thanks for the comments as well!

[Dennis]

Again I’ve updated the original CodePen and included the fade effect (just for show, not really needed). If that function as well can be trimmed down, but yet functioning that would be great! However it’s not needed at this time.

[Josh]

No problem! Your coding for the fadeIn/Out effect looks great, nothing for me to improve there, really.

Best of luck with the rest of the project!

[Author]

Posts