Skip to main content
Code Review

Return to Answer

added 4 characters in body
Source Link
RoToRa
  • 11.6k
  • 1
  • 24
  • 51

I'd like to suggest the opposite of Sᴀᴍ Onᴇᴌᴀ: Since you are using jQuery for selecting only, you can drop it altogether.

const icons = $(".magic");

becomes either

const icons = document.getElementsByClassgetElementsByClassName("magic");

or

const icons = document.querySelectorAll(".magic");

However you shouldn't use on... properties to assign event handlers. on... properties can only hold a single handler so if you or another script would attempt to assign another click handler then they'd overwrite each other. Instead use addEventListener.

Alternatively to avoid assigning seperate event handlers to each icon you could use event delegation. This means assign an single event handler to a surrounding element (or simply document ) and check the target element:

document.addEventListener("click", function (event) {
 if (event.target.classList.contains("magic")) {
 event.target.classlist.toggle("enabled");
 }
};

Finally you could avoid using JavaScript altogether by using an HTML element that has the toggle functionality built in: a checkbox:

<label class="magic-wrapper">
 <input type="checkbox"><span class="magic"><i class="fas fa-star fa-5x"></i></span>
</label>

Hide the actual checkbox with:

.magic-wrapper > input {
 opacity: 0;
 position: absolute;
}

(This is more accessable than just using display: none;)

And replace the selector .enabled with input:checked + .magic in the CSS.

Complete example: https://jsfiddle.net/rhy6gfn4/


A small points about the CSS:

You should select not just .enabled but use .magic.enabled, because then it's more obvious that these rules belong to the animated icons. Also "enabled" is a common class name and you don't want those styles apply to unrelated elements.

It would be a tiniest bit more performant to select the i elements using .magic > i and not just .magic i.

I'd like to suggest the opposite of Sᴀᴍ Onᴇᴌᴀ: Since you are using jQuery for selecting only, you can drop it altogether.

const icons = $(".magic");

becomes either

const icons = document.getElementsByClass("magic");

or

const icons = document.querySelectorAll(".magic");

However you shouldn't use on... properties to assign event handlers. on... properties can only hold a single handler so if you or another script would attempt to assign another click handler then they'd overwrite each other. Instead use addEventListener.

Alternatively to avoid assigning seperate event handlers to each icon you could use event delegation. This means assign an single event handler to a surrounding element (or simply document ) and check the target element:

document.addEventListener("click", function (event) {
 if (event.target.classList.contains("magic")) {
 event.target.classlist.toggle("enabled");
 }
};

Finally you could avoid using JavaScript altogether by using an HTML element that has the toggle functionality built in: a checkbox:

<label class="magic-wrapper">
 <input type="checkbox"><span class="magic"><i class="fas fa-star fa-5x"></i></span>
</label>

Hide the actual checkbox with:

.magic-wrapper > input {
 opacity: 0;
 position: absolute;
}

(This is more accessable than just using display: none;)

And replace the selector .enabled with input:checked + .magic in the CSS.

Complete example: https://jsfiddle.net/rhy6gfn4/


A small points about the CSS:

You should select not just .enabled but use .magic.enabled, because then it's more obvious that these rules belong to the animated icons. Also "enabled" is a common class name and you don't want those styles apply to unrelated elements.

It would be a tiniest bit more performant to select the i elements using .magic > i and not just .magic i.

I'd like to suggest the opposite of Sᴀᴍ Onᴇᴌᴀ: Since you are using jQuery for selecting only, you can drop it altogether.

const icons = $(".magic");

becomes either

const icons = document.getElementsByClassName("magic");

or

const icons = document.querySelectorAll(".magic");

However you shouldn't use on... properties to assign event handlers. on... properties can only hold a single handler so if you or another script would attempt to assign another click handler then they'd overwrite each other. Instead use addEventListener.

Alternatively to avoid assigning seperate event handlers to each icon you could use event delegation. This means assign an single event handler to a surrounding element (or simply document ) and check the target element:

document.addEventListener("click", function (event) {
 if (event.target.classList.contains("magic")) {
 event.target.classlist.toggle("enabled");
 }
};

Finally you could avoid using JavaScript altogether by using an HTML element that has the toggle functionality built in: a checkbox:

<label class="magic-wrapper">
 <input type="checkbox"><span class="magic"><i class="fas fa-star fa-5x"></i></span>
</label>

Hide the actual checkbox with:

.magic-wrapper > input {
 opacity: 0;
 position: absolute;
}

(This is more accessable than just using display: none;)

And replace the selector .enabled with input:checked + .magic in the CSS.

Complete example: https://jsfiddle.net/rhy6gfn4/


A small points about the CSS:

You should select not just .enabled but use .magic.enabled, because then it's more obvious that these rules belong to the animated icons. Also "enabled" is a common class name and you don't want those styles apply to unrelated elements.

It would be a tiniest bit more performant to select the i elements using .magic > i and not just .magic i.

deleted 2 characters in body
Source Link
RoToRa
  • 11.6k
  • 1
  • 24
  • 51

I'd like to suggestivsuggest the opposite of Sᴀᴍ Onᴇᴌᴀ: Since you are using jQuery for selecting only, you can drop it altogether.

const icons = $(".magic");

becomes either

const icons = document.getElementsByClass("magic");

or

const icons = document.querySelectorAll(".magic");

However you shouldn't use on... properties to assign event handlers. on... properties can only hold a single handler so if you or another script would attempt to assign another click handler then they'd overwrite each other. Instead use addEventListener.

Alternatively to avoid assigning seperate event handlers to each icon you could use event delegation. This means assign an single event handler to a surrounding element (or simply document ) and check the target element:

document.addEventListener("click", function (event) {
 if (event.target.classList.contains("magic")) {
 event.target.classlist.toggle("enabled");
 }
};

Finally you could avoid using JavaScript altogether by using an HTML element that has the toggle functionality built in: a checkbox:

<label class="magic-wrapper">
 <input type="checkbox"><span class="magic"><i class="fas fa-star fa-5x"></i></span>
</label>

Hide the actual checkbox with:

.magic-wrapper > input {
 opacity: 0;
 position: absolute;
}

(This is more accessable than just using display: none;)

And replace the selector .enabled with input:checked + .magic in the CSS.

Complete example: https://jsfiddle.net/rhy6gfn4/


A small points about the CSS:

You should select not just .enabled but use .magic.enabled, because then it's more obvious that these rules belong to the animated icons. Also "enabled" is a common class name and you don't want those styles apply to unrelated elements.

It would be a tiniest bit more performant to select the i elements using .magic > i and not just .magic i.

I'd like to suggestiv the opposite of Sᴀᴍ Onᴇᴌᴀ: Since you are using jQuery for selecting only, you can drop it altogether.

const icons = $(".magic");

becomes either

const icons = document.getElementsByClass("magic");

or

const icons = document.querySelectorAll(".magic");

However you shouldn't use on... properties to assign event handlers. on... properties can only hold a single handler so if you or another script would attempt to assign another click handler then they'd overwrite each other. Instead use addEventListener.

Alternatively to avoid assigning seperate event handlers to each icon you could use event delegation. This means assign an single event handler to a surrounding element (or simply document ) and check the target element:

document.addEventListener("click", function (event) {
 if (event.target.classList.contains("magic")) {
 event.target.classlist.toggle("enabled");
 }
};

Finally you could avoid using JavaScript altogether by using an HTML element that has the toggle functionality built in: a checkbox:

<label class="magic-wrapper">
 <input type="checkbox"><span class="magic"><i class="fas fa-star fa-5x"></i></span>
</label>

Hide the actual checkbox with:

.magic-wrapper > input {
 opacity: 0;
 position: absolute;
}

(This is more accessable than just using display: none;)

And replace the selector .enabled with input:checked + .magic in the CSS.

Complete example: https://jsfiddle.net/rhy6gfn4/


A small points about the CSS:

You should select not just .enabled but use .magic.enabled, because then it's more obvious that these rules belong to the animated icons. Also "enabled" is a common class name and you don't want those styles apply to unrelated elements.

It would be a tiniest bit more performant to select the i elements using .magic > i and not just .magic i.

I'd like to suggest the opposite of Sᴀᴍ Onᴇᴌᴀ: Since you are using jQuery for selecting only, you can drop it altogether.

const icons = $(".magic");

becomes either

const icons = document.getElementsByClass("magic");

or

const icons = document.querySelectorAll(".magic");

However you shouldn't use on... properties to assign event handlers. on... properties can only hold a single handler so if you or another script would attempt to assign another click handler then they'd overwrite each other. Instead use addEventListener.

Alternatively to avoid assigning seperate event handlers to each icon you could use event delegation. This means assign an single event handler to a surrounding element (or simply document ) and check the target element:

document.addEventListener("click", function (event) {
 if (event.target.classList.contains("magic")) {
 event.target.classlist.toggle("enabled");
 }
};

Finally you could avoid using JavaScript altogether by using an HTML element that has the toggle functionality built in: a checkbox:

<label class="magic-wrapper">
 <input type="checkbox"><span class="magic"><i class="fas fa-star fa-5x"></i></span>
</label>

Hide the actual checkbox with:

.magic-wrapper > input {
 opacity: 0;
 position: absolute;
}

(This is more accessable than just using display: none;)

And replace the selector .enabled with input:checked + .magic in the CSS.

Complete example: https://jsfiddle.net/rhy6gfn4/


A small points about the CSS:

You should select not just .enabled but use .magic.enabled, because then it's more obvious that these rules belong to the animated icons. Also "enabled" is a common class name and you don't want those styles apply to unrelated elements.

It would be a tiniest bit more performant to select the i elements using .magic > i and not just .magic i.

Source Link
RoToRa
  • 11.6k
  • 1
  • 24
  • 51

I'd like to suggestiv the opposite of Sᴀᴍ Onᴇᴌᴀ: Since you are using jQuery for selecting only, you can drop it altogether.

const icons = $(".magic");

becomes either

const icons = document.getElementsByClass("magic");

or

const icons = document.querySelectorAll(".magic");

However you shouldn't use on... properties to assign event handlers. on... properties can only hold a single handler so if you or another script would attempt to assign another click handler then they'd overwrite each other. Instead use addEventListener.

Alternatively to avoid assigning seperate event handlers to each icon you could use event delegation. This means assign an single event handler to a surrounding element (or simply document ) and check the target element:

document.addEventListener("click", function (event) {
 if (event.target.classList.contains("magic")) {
 event.target.classlist.toggle("enabled");
 }
};

Finally you could avoid using JavaScript altogether by using an HTML element that has the toggle functionality built in: a checkbox:

<label class="magic-wrapper">
 <input type="checkbox"><span class="magic"><i class="fas fa-star fa-5x"></i></span>
</label>

Hide the actual checkbox with:

.magic-wrapper > input {
 opacity: 0;
 position: absolute;
}

(This is more accessable than just using display: none;)

And replace the selector .enabled with input:checked + .magic in the CSS.

Complete example: https://jsfiddle.net/rhy6gfn4/


A small points about the CSS:

You should select not just .enabled but use .magic.enabled, because then it's more obvious that these rules belong to the animated icons. Also "enabled" is a common class name and you don't want those styles apply to unrelated elements.

It would be a tiniest bit more performant to select the i elements using .magic > i and not just .magic i.

lang-css

AltStyle によって変換されたページ (->オリジナル) /