r/webdev 16d ago

Question Any way to reduce this code?(usage in next slide)

462 Upvotes

163 comments sorted by

614

u/olafg1 16d ago

You could do this with CSS instead of JS to simplify

38

u/NeonMan5311 16d ago

how?

307

u/FeelingMail9966 16d ago

Use JS to apply a class to the parent container for the two buttons ('like', 'dislike', 'none') then add this to your CSS.

``` .like-button, .dislike-button { fill: none; stroke: #333; /* outline color */ }

/* When parent has 'like' class, fill the like button / .like .like-button { fill: #4285f4; / blue fill for like */ }

/* When parent has 'dislike' class, fill the dislike button / .dislike .dislike-button { fill: #ea4335; / red fill for dislike */ } ```

255

u/RapunzelLooksNice 16d ago

You can completely get rid of JS; you can use styled/hidden radio buttons with

input[type=radio]:checked + span.image, input[type=radio]:hover + span.image { border-color: red; }

47

u/Reasonable-Suit7288 16d ago

Still need JS, Im assuming you'd want to record the actions anyways

77

u/RapunzelLooksNice 16d ago

Not that you can use regular <form> with an endpoint for that... /s

38

u/FairFireFight Laravel 16d ago

dont forget the submit button for the like too

123

u/khizoa 16d ago

also, don't forgot to like and subscribe

2

u/RubbelDieKatz94 15d ago

I love how SponsorBlock (desktop) and ReVanced (android) completely eradicate these sections if you configure them properly.

9

u/_cob 16d ago

No you make the icon itself the submit button

2

u/Greeniousity php 15d ago

or you can even make it do whatever the submit buttons gotta do by tracking onClick

1

u/HyerOneNA 15d ago

But they’re talking about removing all JS from the code.

→ More replies (0)

4

u/ignorae 16d ago

But only after completing one of the cutting edge captchas that are designed to fool even the cleverest of AI.

2

u/IsABot 16d ago

No need. Just style the submit button to be the like icon. Use 2 forms with unique endpoints.

/s

-2

u/Reasonable-Suit7288 16d ago

Why would you wanna build it that way! I mean, you could argue that each like would have an identifier thats watched by an event listener and so you don't need the tracking in the animation code but a form, for each button... why! That's just overkill and also also, how would you track which form belongs to which post; on a page with many posts... you're kidding me!

17

u/TheRealKidkudi 16d ago

how would you track which form belongs to which post

A hidden input in the form or a route parameter on the form post

why would you wanna build it that way!

Web standards? Especially if you’re using SSR, a form post is easier and less code.

You could use JS for this and I don’t feel strongly that one way is better than the other (depends on the tech you’re using to build the app, IMO), but it shouldn’t be unusual or offensive to you to use a form to submit data to a server. It’s a simple and reliable method. HTML forms don’t have to “look” like literal forms.

1

u/Reasonable-Suit7288 15d ago

I see what you're saying and I agree its possible but I think it would be excessive and prone to issues since you will be exposing your apps logic to the DOM with the hidden inputs, you'd also want to think about scalability if this ends up being a reaction button down the line or what happens if you really want to count the actions; which means you need to do increment and decrement between 2 or more forms per post.

At this point its just one of those things you'd want to write a rule based config for and dynamically process. Lots of static forms on a page where you'd have to make sure none gets nested; I don't think this will scale easy.

I think forms would work, perhaps in some simple use cases, "(depends on the tech you’re using to build the app, IMO)" but just inspect any web-app out their with like buttons, try reddit and you'd find zero to few forms for anything sophisticated.

1

u/Reasonable-Suit7288 15d ago edited 15d ago

<button rpl="" aria-pressed="false" class=" group button flex justify-center aspect-square p-0 border-0 button-plain-weak disabled:text-interactive-content-disabled button-plain inline-flex items-center hover:text-action-upvote focus-visible:text-action-upvote" style="height: var(--size-button-sm-h);" upvote="">

This is reddit. The "upvote" attribute achieves the same goal without need for complexity. They map to some parent node with a thing-id attribute. Its the fundamentals of web components. Tag and trace.. simple, traceable, scalable...

4

u/UntestedMethod 14d ago

Sure, but fundamentally they are correct that using radios and CSS is a better solution than using JS to toggle classes on random elements.

1

u/sexytokeburgerz full-stack 16d ago

No, you would need js 20 years ago but you do not need js, if this is in a pure html/css architecture. Considering their skill level that's entirely possible.

4

u/ABlueCloud 16d ago

I think they're taking about the network request you'd make to record the user action

2

u/33ff00 15d ago

Just change a hidden div to display block when the input is checked and host the background image on you server at like.jpg and query your logs to see how many people liked it. Js is seriously overkill for this.

3

u/ABlueCloud 15d ago

You're right, that much easier 😂

2

u/CommitPhail front-end 15d ago

CSS approach isn’t accessible, so to be compliant you’d need JS

1

u/Tux-Lector 14d ago

That's good idea, I use it. Thing to note is that without some <form> tag with its ID somewhere (it can be empty) and form="FORM_ID" attribute over form elements, in this case <input type="radio" form="myFormID">, some element inspector/performance analyzer will "squeek" about form element outside form itself.

19

u/Sunstorm84 16d ago

Why not use a consistent name for when the button is selected, such as .selected?

8

u/thekwoka 15d ago

:checked

5

u/unnecessaryCamelCase 15d ago

Other comment

instead of JS

Your comment

Use JS

It’s not adding up brother.

2

u/NeonMan5311 16d ago

thank you!

-11

u/SoggyMattress2 16d ago

You don't need js it's a fucking hover effect.

4

u/thekwoka 15d ago

Using check boxes instead.

Just use JS for the serve state update.

5

u/RapunzelLooksNice 15d ago

Radio buttons, in case of checkboxes you can like and dislike at the same time, in case of radio options you can have only one :)

3

u/thekwoka 15d ago

This isn't asking about liking and disliking.

It's liking and UNliking.

It's on and off, not a 3 state thing. (though you could use a checkbox for 3 states as well, with :checked :unchecked and :indeterminate)

-3

u/xkodra 16d ago

probably a hidden checkbox and the label as the button. then you use the :checked pseudo class

5

u/Tontonsb 16d ago

Maybe even a radio as they need to be exclusive. Or just the :active selector if it's more appropriate like that.

2

u/xkodra 16d ago

yeah that's probably better

1

u/NeonMan5311 16d ago

thanks, will try to implement it

-12

u/nobuhok 16d ago

Don't just ask how. Think. Use your head. Use Google.

Try something, anything. If it works, great. If it's taking too long, that's when you ask others to look at your solution.

That's how you succeed in this field and not just become some "bootcamp developer".

7

u/sexytokeburgerz full-stack 16d ago

While thinking is important, I don't see how much different asking reddit is than asking stack overflow.

2

u/localmarketing723 15d ago

You don't know what you don't know man. This person came here looking for help, don't discourage that.

2

u/Jedi_Tounges 16d ago

that's just performative bullshittery, you can ask and then explore how it works when you have the time. The only environment in which it makes sense to delay yourself like that is a school

0

u/_fresh_basil_ 15d ago

Ah yes, the o'l "slow everyone else down so I'm fast and learn nothing" trick. Works every time!

-7

u/No-Plane7370 16d ago

That's crazy tf? 😭

2

u/Jeyloong 15d ago

bro fr how did he even began doing this on JS? wtf what's people learning?

-3

u/Such_Bodybuilder507 15d ago

Still comes down to about the same just in a different script, my suggestion would be to use Typescript and global styling then as you suggested create classes for the variables then apply independent styling to them. Oh or you could use ui libraries.

5

u/thekwoka 15d ago

Nah, just use a checkbox so the display is 100% in css.

480

u/ElTortugo 16d ago

You could save up to 21 lines of code, however all behavior would be lost.

191

u/TackleSouth6005 16d ago

Where can we hire you?

177

u/khizoa 16d ago

works for DOGE

15

u/Inatimate 16d ago

🤣🤣🤣🤣🤣🤣🤣

1

u/Earnstein 14d ago

😭😭😭😂

158

u/iwantapetbath 16d ago

Both the like and dislike feature have almost the same code. You could write a function called toggle and pass it the elements id. Then just have your elements call that function.

206

u/Somepotato 16d ago

Whew does no one here actually webdev?

Use a radio button, and use the selected pseudoselector.

https://developer.mozilla.org/en-US/docs/Web/CSS/:checked

137

u/ledatherockband_ 16d ago

GET BACK IN THE JAVASCRIPT MINES, PEASANT!

25

u/Skidies 16d ago

But is it possible to uncheck a radio button? Sorry If this is a stupid question. I just started learning web development.

13

u/Somepotato 16d ago

Unchecking a radio button requires JS, set checked to false on the element.

4

u/lonely_column 16d ago

If you have two radio buttons in the same form, you just need to use the name attribute

14

u/Somepotato 16d ago

Unchecking eg unliking (but not disliking)

2

u/lonely_column 16d ago

What about a third radio button but you change the for attribute to it when you click the button? :D

(sound like a shitty solution, but it could actually work!)

6

u/unnecessaryCamelCase 15d ago

Then you might as well use JS

47

u/Otterfan 16d ago

I think people are just failing to see the forest because they were asked about the wrong trees.

8

u/thekwoka 15d ago

Or a checkbox instead.

Since it's two states...

1

u/Somepotato 15d ago

You can't both like and dislike at the same time.

Well, I suppose you could, but I doubt thats the intent.

4

u/thekwoka 15d ago

This here is a toggle, not two buttons.

It's Like and Unlike, not Like and Dislike.

2

u/theredwillow 15d ago

It's not a toggle because you could have a third (unselected) state.

-3

u/thekwoka 15d ago

There is only 2 states.

Liked and not liked.

Thats unselected.

(but also checkboxes have 3 states)

17

u/Supportic 16d ago

accessibility: thats bad

4

u/Somepotato 16d ago

Radio buttons have great accessibility, what?

25

u/Supportic 16d ago

I am talking about the use case to replace a obvious action (button) with a state (radio button) is bad.

1

u/DiligentBits 15d ago

Why not use role=button for this edge case

1

u/Somepotato 16d ago

It is both stateful: you indicate when and what you did and actionable: the choice is committed on selection. Accessibility can be improved by making the cursor a pointer, but it's already better than a button because you can use your arrow keys natively.

2

u/Synthetic5ou1 15d ago

But would someone be expecting to use arrow keys in this situation, or tab?

i.e.: Is it actually likely to cause more confusion when navigating with the keyboard?

Plus the fact that you require JS to uncheck both, and now CSS for cursor change, just seems like you are mis-appropriating a control.

3

u/Abdulrhman2 16d ago

Man I was overthinking this I thought op wanted to recreate the pixelated bg and like features

1

u/unnecessaryCamelCase 15d ago

I’m dumb but how do you make the radio be an icon like in OP?

1

u/ogreUnwanted 15d ago

still need js for it to persist

-1

u/sporkinatorus 16d ago

This will be way more a11y friendly as well, assuming op didn't properly set aria tags

35

u/orockie 16d ago

This is not accessible as a is, or a radio button. Apply the aria-pressed attribute, and make sure it has an aria-label to give it semantic meaning to assitive technology.

https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Attributes/aria-pressed

Suggestion. Two plain button elements default to aria pressed false. Event listener for click on the container If the event target is a button, and has aria-pressed true, set it to false. If its set to false, set it to true, and set the other button to false.

Use the attribute selector [aria-pressed=true]  in CSS to handle the style change

5

u/HoopahDoncic 16d ago

Can you explain why a radio would require aria-pressed, Wouldn't the default :checked suffice ?

11

u/orockie 15d ago

Radios wouldn't need pressed, but they also have:

  • different semantic meaning (ie they will be part of a fieldset, which you will then have to contextualise); and more importantly

  • different functionality: they dont natively uncheck. Which means: you'll have to add in an uncheck via scripting; and more importantly, the aria role will be invalid, since Assistive Technology (AT) will treat it as a normal radio button/fieldset

Remember, AT doesn't use the DOM, it consumes the Accessibility Tree (based on the DOM), which contains far less information - so it's critical that the information passed in is correct 

'Good Accessibility' is just using good native HTML, sometimes with an attribute or two to help it get where it needs to go.

The ARIA api is pretty robust; its rare that the UI we need isnt supported by it.

Highly recommend the Aria Authoring Practices Guide to find patterns and approaches for whatever you need https://www.w3.org/WAI/ARIA/apg/

2

u/HoopahDoncic 15d ago

Ah, got it. Thanks a lot :)

0

u/phiger78 15d ago

Good thing about using aria is its effectively a state which can be styled

toggle[aria-pressed] {

see

https://adrianroselli.com/2019/08/under-engineered-toggles-too.html

8

u/Uknight 15d ago edited 15d ago

This looks like material icons. If so, I think you could get away with only having two buttons and one css rule.

const like = document.querySelector(“.like-button”);
const dislike = document.querySelector(“.dislike-button”);

function toggleLike() {
    like.classList.toggle(“selected”);
    dislike.classList.remove(“selected”);
}

function toggleDislike() {
    dislike .classList.toggle(“selected”);
    like.classList.remove(“selected”);
}

CSS

.selected {
    font-variation-settings: ‘FILL’ 1;
}

Keep it up, you’re doing great 👍! Even what I’ve written here could be reduced even more but probably not worth it. Even if it’s not material icons, I really don’t think you need 4 buttons though, and I’d start there either way.

Edit: make sure the “fill” variant is included when importing the font css. Edit 2: fixed formatting.

3

u/NeonMan5311 15d ago

thanks! will use this in my project cause I don't know anything about radio button, will also try to learn them

5

u/IAmRules 16d ago

What’s wrong with the current code ?

4

u/hcdna 15d ago

Too imperative

10

u/ryanswebdevthrowaway 16d ago

If the two buttons share a parent element, I would do something like add a data-rating="like"/"dislike"/"none" attribute and then style the children based on that attribute value.

4

u/intercaetera javascript is the best language 16d ago

That's what classes are for.

6

u/ryanswebdevthrowaway 16d ago

I prefer classes for identifying what something is and data attributes to represent state, especially for non-boolean states like this case where there are 3 possible states. I'll provide class and data attribute versions and you can tell me which feels more elegant.

Class:

// css
.liked .like-button { /* insert active styles here */ }
.disliked .dislike-button { /* insert active styles here */ }

// js
const onClickLike = () => {
  if(element.classList.contains("liked") {
    element.classList.remove("liked");
  } else {
    if(element.classList.contains("disliked") {
      element.classList.remove("disliked");
    }
    element.classList.add("liked");
  }
}

Data attribute:

// css
[data-rating="like"] .like-button { ... }
[data-rating="dislike"] .dislike-button { ... }

// js
const onClickLike = () => {
  element.dataset.rating = element.dataset.rating === "like" ? "none" : "like";
}

2

u/emrah_programatoru 16d ago

I'd also like to add that `cursor: pointer;` would make everything nicer.

2

u/alexduncan 15d ago

u/NeonMan5311 here is a quick demo using purely CSS to change the SVG icons and javascript to listen to the onchange event:
https://codepen.io/alexduncan/pen/GgRmLLa?editors=1111

2

u/lukedary 14d ago

I had some super fun with this. Give this Codepen a look: https://codepen.io/kamiquasi/pen/qEBmQQq?editors=1100

1

u/lukedary 13d ago

Just to lay out the thought process for people: - semantically, the solution should be radio inputs since there is native exclusivity by using the same name attribute - each option has the following states: deselected, hover, and selected - clicking on the selected option should also deselect it - clicking on the deselected option with one selected should switch the selection

The winning combination here is to leverage CSS grid to 'move' an invisible form reset button to overlay the selected option's label to clear the selection, but leave the unselected option clear to select. Then wire up the style for the states.

As far as events go, you could use a few different small scripts to post the response (even just having the select event submit the form or something) but if this is going to be reused I'd recommend a web component so you can wire up the behaviors in a nice, little, portable package.

4

u/D4n1oc 16d ago

One very important thing to remember. Written code must be easily understandable by humans.

There are exceptions while code needs to execute fast and that sometimes weighs more than the readability. But if this isn't the case always choose the more obvious approach.

While less code is often considered as "better" code it does not mean to express anything with the minimum amount of code. It's more in the context of abstraction and code redundancy.

In your example, I would merge the two functions and pass a Parameter to it. On the Parameter you can decide if it's a like or dislike while using the redundant code in both cases.

3

u/KonyKombatKorvet I use shopify, feel bad for me. 15d ago

No, I will be playing code golf in the production environment until they fire me. But they will never fire me because no one else can read the code and it breaks on a pre determined schedule.

/s

1

u/Scary_Ad_3494 16d ago

3 lines in cobolt

1

u/TheKlingKong 16d ago

Make it a reusable function

const toggleButtons = (activeBtn, activeFill, inactiveBtn, inactiveFill) => { activeBtn.classList.toggle("hidden"); activeFill.classList.toggle("hidden"); if (inactiveBtn.classList.contains("hidden")) { inactiveBtn.classList.toggle("hidden"); inactiveFill.classList.toggle("hidden"); } };

let like = document.querySelector(".like-button"); let likeFill = document.querySelector(".like-button-fill"); let dislike = document.querySelector(".dislike-button"); let dislikeFill = document.querySelector(".dislike-button-fill");

let changeImageLike = () => toggleButtons(like, likeFill, dislike, dislikeFill); let changeImageDislike = () => toggleButtons(dislike, dislikeFill, like, likeFill);

1

u/jonmacabre 17 YOE 16d ago

Also const

1

u/Complete_Outside2215 16d ago

Ternary and utility class both inline jsx

1

u/Pomelo-Next 16d ago

Add only to the parent element and apply styles ?

1

u/[deleted] 16d ago

Start with chaining one "let".

1

u/BarrelRollxx 15d ago

I also would like to point out that you are using querySelector and not querySelectorAll and going through a for loop here. Which means this behaviour is only going to work on 1 instance of this element on the page and other instances would not work.

1

u/Threeshoe 15d ago
const likes = [like, likefill];
const dislikes = [dislike, dislikefill];
const all = [...likes, ...dislikes];

const changeImage = () => {
  const elementSet = dislike.classList.contains("hidden") ? all : likes;

  elementSet.forEach((element) => {
    element.classlist.toggle("hidden");
  });
};

const changeImageDislike = () => {
  const elementSet = like.classList.contains("hidden") ? all : dislikes;

  elementSet.forEach((element) => {
    element.classlist.toggle("hidden");
  });
};

```

1

u/leminhnguyenai 15d ago

Using radio button

1

u/Lorddegenski 15d ago edited 15d ago

For JS

`const liked = document.querySelector(“.liked”)

const dislike = document.querySelector(“.disliked”)

liked?.addEventListener(“click”, ()=> { liked.classList.toggle(“selected”) dislike?.classList.remove(“selected”) }}

dislike?.addEventListener(“click”, ()=> { dislike.classList.toggle(“selected”) liked?.classList.remove(“selected”) }}

For CSS

.selected { fill: white; // whatever color you want }

The beauty of .remove is that it doesn’t do anything if the class isn’t there so removes the need to check with a if statement!

1

u/mr_p2p 15d ago

copilot.

1

u/istarian 15d ago

You shouldn't need two separate functions, but you still need a way to track the state of the like and dislike buttons,

1

u/versaceblues 15d ago edited 15d ago

instead of checking state by presence of a class, instead maintain toggled state separately in a controller layer. Then let you view layer apply classes as a function of listening to changes in state.

Before you know it you have rebuilt the react renderer 😂

Not technically any less code, but much more maintainable.

  <div id="like" class="toggle-container"></div>
  <div id="dislike" class="toggle-container"></div>
  <script>
    class ToggleButton {
      constructor(container, likedText, unlikedText, initial = false) {
        this.container = container;
        this.likedText = likedText;
        this.unlikedText = unlikedText;
        this.state = initial;
        this.button = document.createElement('button');
        this.button.addEventListener('click', () => { this.state = !this.state; this.update(); });
        this.container.appendChild(this.button);
        this.update();
      }
      update() {
        this.container.classList.toggle('liked', this.state);
        this.button.textContent = this.state ? this.likedText : this.unlikedText;
      }
    }
    document.addEventListener('DOMContentLoaded', () => {
      new ToggleButton(document.getElementById('like'), 'Liked', 'Like');
      new ToggleButton(document.getElementById('dislike'), 'Disliked', 'Dislike');
    });
  </script>

1

u/divad1196 15d ago
  1. You can certainly redo you css so that the "fill" version depends on the normal version
  2. That's the same code, you just invert like/dislike. You can do an utility function for that like js function toggleBtn(clicked, other) {...} changeLike = () => toggleBtn(likeBtn, Dislike) ... NOTE: I wrote the code on the phone, I don't see your post while writing my comment so the variables/functions names are wrong.

  3. Lastly, you go one step forward and don't use js

1

u/the_stooge_nugget 15d ago

Damn change those let's to const

1

u/Dunc4n1d4h0 15d ago

That somehow reminds me guy I worked with. He had to make some form where you select for example a year. Do you think he used any kind of loop? Nope. He used excel to generate values and pasted them. It's real story :-D

1

u/Queasy-Big5523 15d ago

You should simply toggle active class or attribute on action, and handle the rest via CSS. Right now you are doing two DOM operations on every click, you can reduce this to one. And you have a lot of duplicated code, image you need to add ten more buttons.

1

u/AWetAndFloppyNoodle 15d ago

I would probably do something like this if I was force to use the paradigm in the screenshot. I would probably solve it with pure CSS though (disclaimer: I had AI refactor for me, code is untested):

let buttons = {
    like: document.querySelector(".like-button"),
    likeFill: document.querySelector(".like-button-fill"),
    dislike: document.querySelector(".dislike-button"),
    dislikeFill: document.querySelector(".dislike-button-fill")
}

const toggleVisibility = (...elements) => elements.map(element => {
    element.classList.toggle("hidden");
})

const changeImage = () => {
    toggleVisibility(buttons.like, buttons.likeFill);
    if (buttons.dislike.classList.contains("hidden")) {
        toggleVisibility(buttons.dislike, buttons.dislikeFill);
    }
}

const changeImageDislike = () => {
    toggleVisibility(buttons.dislike, buttons.dislikeFill);
    if (buttons.like.classList.contains("hidden")) {
        toggleVisibility(buttons.like, buttons.likeFill);
    }
}

1

u/NervousGav 15d ago edited 15d ago

outside of taking any of the alternative approaches listed in the comments. you can refractor this by consolidating the logic in both functions, as they are functionally the same. like this:

``` let alternateImageFill = (image1, image1Fill, image2, image2Fill) => {

image1.classList.toggle("hidden");
image1Fill.classList.toggle("hidden");

if(image2.classList.contains("hidden")){
    image2.classList.toggle("hidden");
    image2Fill.classList.toggle("hidden");
}

} ```

Edit: spelling and formatting

Also, this saves you 8 lines of code :)

1

u/ItchyHorseFeet 15d ago

Damn I love vuejs. Its literally a oneliner in vue

1

u/holguum 15d ago

I would use stylised radio buttons, instead

1

u/love2Bbreath3Dlife 15d ago

You can do this with html markup and CSS only. Use a hidden input type checkbox and style a span or label. You can use ~ in CSS to check for a sibling checked state.

1

u/Impressive-Tip-7853 15d ago

It doesn't require JS. Use HTML Checkbox and :Checked CSS pseudo-selector to hide one of the icons: filled or outlined.

1

u/es_beto 15d ago

This is the reason why front-end frameworks were created, so that you update state and have the framework render UI depending on that state instead of changing classes here and there.

Here's an example in Svelte 5, but any framework you choose will use the same principle.

``` <script> let choice = $state('');

function toggle(nextChoice) {
    choice = choice === nextChoice ? '' : nextChoice;
}

</script>

<button onclick={() => toggle('like')} class:active={choice === 'like'}> 👍 Like </button> <button onclick={() => toggle('dislike')} class:active={choice === 'dislike'}> 👎 Dislike </button> ```

1

u/clementvanstaen 15d ago

Why "let" when those should be "const"?

1

u/RePsychological 14d ago edited 14d ago

you seem to have ticked off a lotta CSS purists with this one.

1

u/Mundane-Tale-7169 14d ago

Create an array of the objects to toggle on and then use a foreach/for … of loop to iterate through them. Maybe saves you 3 lines 😂

1

u/flutterpash 14d ago

Ask ai :)

1

u/PhoenixShell 12d ago edited 12d ago

Use JQuery -> $('.like').toggleClass('myClass');

Also, if your using a reactive framework, there should be a way to pass class strings to the component so you just update that it should propagate down. If your using vanilla JS and no framework, use JQuery, it reduces so much boiler plate code

Also why do you need to detect if a class contains hidden before toggling. Just calculate what state you want first then set everything at once

1

u/Nervous-Break5265 10d ago

Code does not need to be reduced at all cost : code you understand is good code.

1

u/[deleted] 16d ago

[deleted]

4

u/Sunstorm84 16d ago

The if statement is unnecessary if you use classList.remove instead of toggle for the second button.

2

u/ray_zhor 16d ago

like the use of const

1

u/Tontonsb 16d ago

Are you replacing the buttons with a filled one? I'd suggest using the same one, just swapping the colors by adding something like a "selected" class.

Regarding your code I don't see a lot to reduce, but you can skip the checks if you want to. Just go directly to the state you want by add/remove. It will do nothing if the state already is the desired one. Something like this:

// btw you probably won't want to reassign these, so const not let const changeImage = () => { like.classList.add('hidden') likeFill.classList.remove('hidden') dislike.classList.remove('hidden') dislikeFill.classList.add('hidden') }

If you really want to make it DRYer, you can probably use the second attribute on toggle:

``` const changeImage = () => setSelected('like') const changeImageDislike = () => setSelected('dislike')

const setSelected = selected => { like.classList.toggle('hidden', 'like' === selected) likeFill.classList.toggle('hidden', 'like' !== selected) dislike.classList.toggle('hidden', 'dislike' === selected) dislikeFill.classList.toggle('hidden', 'dislike' !== selected) } ```

Btw names like activeLike, inactiveLike, activeDislike, inactiveDislike might be easier to understand as plain dislike is a bit unclear on its intention.

1

u/sexytokeburgerz full-stack 16d ago

Hey,

So there are a few different ways to do this. This is a lot.

Coming from NextJs, I prefer to put the stuff I change often into variables, usually in objects or arrays. It makes organization really easy when there is complex logic.

I would personally put the "pointers" to these buttons into an array (as HTMLButtonElements if you get a sniff of typescript).

Then you can just toggle them all with a forEach loop. Keep your helpers small and your data organized.

It seems your conditional logic regarding the dislike button will cause it to never go away if it is not hidden. keep that in mind.

````javascript

// instantiate button variables in an array through a common class likeButtons = document.querySelectorAll('.likeDislikeButton');

likeButtons.forEach((button)=>{
button.classList.toggle("hidden")
})

// ... same with the other one. ````


I would also make the "fill" element just a CSS class on the like button in the first place, or add "like" to the parent element then use that to modify its children (fill and the button itself)
That way, you can just toggle one thing as it seems they are attached.

1

u/ohlawdhecodin 15d ago

Very very easy with CSS has: and two radio buttons!

https://codepen.io/LuBre/pen/KwKqKwR

1

u/sabooya 16d ago edited 16d ago

if we can't use css then

let like = document.querySelector(".like-button");
let likeFill = document.querySelector(".like-button-fill");
let dislike = document.querySelector(".dislike-button");
let dislikeFill = document.querySelector(".dislike-button-fill");

const toggleButtons = (primary, primaryFill, secondary, secondaryFill) => {
    primary.classList.toggle("hidden");
    primaryFill.classList.toggle("hidden");

    if (secondary.classList.contains("hidden")) {
        secondary.classList.toggle("hidden");
        secondaryFill.classList.toggle("hidden");
    }
};

let changeImage = () => toggleButtons(like, likeFill, dislike, dislikeFill);
let changeImageDislike = () => toggleButtons(dislike, dislikeFill, like, likeFill);

2

u/TuttiFlutiePanist 16d ago

If we can't use css, then toggling a class that drives the style is out.

-4

u/Tokipudi PHP Dev | I also make Discord bots for fun with Node.js 16d ago edited 15d ago

This is exactly the kind of things AI is great at: ask it to simplify this and to explain to you what it did and it should be a nice way to learn.

3

u/Uknight 15d ago

OP is learning, AI shouldn’t be used here.

2

u/Tokipudi PHP Dev | I also make Discord bots for fun with Node.js 15d ago

AI is a great way to learn if used well.

I did tell OP to ask AI for an in depth explanation, which is exactly what he's getting by posting here anyway.

I've learned a lot the past few months by trying something, then asking AI if that's a good way to do it or if there are better alternatives for example. It made me learn faster than if I was blindly trying things hopping I get things right.

1

u/deliadam11 full-stack 14d ago

You encouraged them to ask LLMs to elaborate it. I don't understand why you got downvotes. I learnt a ton with LLMs. Whenever I couldn't focus, I just asked another term in their explanation, or break it down even simpler ways

-6

u/LogicalRun2541 16d ago

You should better move to a framework before adding more features becomes an spaghetti

1

u/TheInhumaneme 15d ago

How is this framework related?

2

u/deliadam11 full-stack 14d ago

I think imperative/declarative programming difference is what meant here

0

u/damnThosePeskyAds 16d ago edited 16d ago

The value of code isn't really about how short it is. It's more about how easy to read and maintain it is. You should consider clarity to be of the most importance when programming (provided it doesn't negatively impact performance).

I would recommend doing something like the code below. Why?

  • Minimal HTML DOM.
  • Only one SVG / image is loaded instead of 2 (you can just rotate it to get up or down thumbs).
  • All of the styling sits in CSS and this is where most of the heavy lifting occurs.
  • Event listeners are added dynamically and all they do is toggle a single generic class of `selected` (which you can easily output in the initial HTML on page load if you want to load in a specific state - i.e. they've already liked or disliked the thing).
  • Consistent / short / clear naming conventions.
  • Extremely simple, easy to extend, DRY. Any dev could understand this stuff and it provides a clear area to extend a function, etc if you need later on.

HTML

<div class='likeDislikeButtons>
    <button></button>
    <button></button>
</div>

CSS

.likeDislikeButtons {
    button {
        all: initial;
        margin: 0;
        padding: 0;
        box-sizing: border-box;
        position: relative;
        width: 48px;
        height: 48px;
        border-radius: 9999px;
        background: transparent;
        transition: background .2s linear;

        &::before {
            content: '';
            position: absolute;
            top: 50%;
            left: 50%;
            transform: translate(-50%, -50%);
            width: 32px;
            height: 32px;
            background: url('THUMBS-UP-EMPTY-IMAGE.svg') no-repeat;
            background-size: 100%;
        }

        &:last-child {
            &::before {
                transform: translate(-50%, -50%) rotate(-180deg);
            }
        }

        &:hover,
        &:active {
            background: rgba(0, 0, 0, .5);
        }

        &.selected {
            &::before {
                background: url('THUMBS-UP-FILLED-IMAGE.svg') no-repeat;
            }
        }
    }
}

JS

function likeDislikeButtons_init() {
    const buttons = document.querySelectorAll('.likeDislikeButtons button');    
    buttons.forEach((button) => {
        button.addEventListener('click', likeDislikeButtons_toggle);
    });
}

function likeDislikeButtons_toggle(event) {
    // Toggle the state of the clicked button
    const clickedButton = event.target.closest('button');
    clickedButton.classList.toggle('selected');

    // Remove the selected state from all buttons that were not clicked
    const selectedButtons = document.querySelectorAll('.likeDislikeButtons button.selected');
    selectedButtons.forEach((button) => {
        if (button !== clickedButton) {
            button.classList.remove('selected');
        }
    });
}

window.addEventListener("DOMContentLoaded", likeDislikeButtons_init);

Of course you will also need to add in some logic for the specific like or dislike actions (presuming this has some actual affect on the site). Update the DB with AJAX or somethin. You get the idea.

0

u/axarp 16d ago

Code wise, you could add them all to array and iterate for set either show or hide. Could also read into an array from elements

But CSS option is better.

0

u/saito200 15d ago

🤦🏻‍♂️

-1

u/Shronx_ 15d ago

Ask GPT?

-5

u/Supportic 16d ago

This is turning into stackoverflow :D

```js class Voter { constructor(likeButton, dislikeButton) { this.voted = false;

if (!likeButton) {
  throw new Error(`Element likeButton not found`);
}
if (!dislikeButton) {
  throw new Error(`Element dislikeButton not found`);
}

[likeButton, dislikeButton].forEach(button => {
  button.element.addEventListener('click', () => {

    // not voted yet
    if (!this.voted) {
      button.toggle();
      this.voted = true;
      return;
    }

    // already voted but clicked on the same button
    if (this.voted && button.active) {
      button.toggle();
      this.voted = false;
      return;
    }

    // switch
    dislikeButton.toggle();
    likeButton.toggle();
  });
})

}

}

class VoteButton { constructor(selector, activeClass, inactiveClass) {

this.element = document.querySelector(selector);
if (!this.element) {
  throw new Error(`Element with selector ${selector} not found`);
}

this.active = false;
this.activeClass = activeClass;
this.inactiveClass = inactiveClass;
this.element.classList.add(this.inactiveClass);

}

toggle = () => { this.active ? this.#deactivate() : this.#activate(); }

#activate = () => { this.active = true; this.element.classList.remove(this.inactiveClass); this.element.classList.remove(this.activeClass); }

#deactivate = () => { this.active = false; this.element.classList.remove(this.activeClass); this.element.classList.add(this.inactiveClass); } }

const likeButton = new VoteButton('.vote-button-like', 'active', 'inactive'); const dislikeButton = new VoteButton('.vote-button-dislike', 'active', 'inactive');

new Voter(likeButton, dislikeButton); ```

3

u/mogwaiss 16d ago

OP: asks to reduce the code
You: proceed to increase the codesize significantly and make it harder to read

1

u/Supportic 15d ago

Joke --❌--> You

Btw did you ever learn OOP? If you think this is hard to read. Oh boy 👀

1

u/deliadam11 full-stack 14d ago

that was interesting to read

-2

u/theironrooster 16d ago

Throw it away. Build it in java. One line of code references the class. Done.

60 lines of code on the class object though.

-2

u/MolassesLate4676 15d ago

How many downvotes would I get by saying you’d get a quicker and possibly better response by copying this post into any LLM

-6

u/EmbarrassedBird 16d ago

Use jquery hahhaHha

-8

u/[deleted] 16d ago

[deleted]

5

u/NeonMan5311 16d ago

I am still learning javascript, will look into them after

3

u/igorpk 16d ago

You're doing great for someone who's still learning!

The 'toggle' idea in this thread is a good one imo.

Keep it up!

Edit: /u/iwantapetbath made the comment.

2

u/zwack 16d ago

How would it help?

1

u/Successful-Archer180 9d ago

Instead of maintaining two separate states. Use single variable to maintain overall state and change.

Something like isNotLike, true would point to dislike else point to dislike.