I am new to Javascript and trying to create a Javascript click function that will display the input element when the button is clicked and hide it when the button is clicked again. My console shows no error messages but when I click the button the input element does not hide. Again I am very new to Javascript, I appreciate any feedback!
document.addEventListener("DOMContentLoaded", load);
function load() {
let button = document.querySelector("button");
button.addEventListener("click", clickMe);
}
// Click Function to change the display value of the input
function clickMe() {
let input = document.getElementById("popup");
if (input.style.display == "none") {
input.style.display = "block";
} else {
input.style.display = "none";
}
}<!-- <form> I commented out the form element because it does not work to use .addEventListener inside form -->
<label for="button"></label>
<fieldset>
<ol>
<li><button type="button" onclick="clickMe()">Click me!</button></li>
<li><input type="text" name="popup" id="popup" placeholder="placeholder"></li>
</ol>
</fieldset>
<!-- </form> -->You are adding the click listener twice, in HTML with onclick and in JavaScript with addEventListener(). Therefore it is executed twice for each click.
Since your listener toggles between two states, calling it twice behaves the same as calling it none at all. This is the issue you are observing.
Adding the listener only once solves this issue. Prefer to keep addEventListener(); reasons are stated later on.
Since you mentioned that you would "appreciate any feedback", I collected a few points that may be of interest:
"DOMContentLoaded"In StackOverflow snippets, the script is always added as <body>'s last element, however in other scenarios this can not be ensured.
Scripts can be added in many different ways to pages:
type="module" scripts.defer.async.type="module" scripts.Additionally, for regular non-deferred scripts, position of the script element in the document is also relevant.
But by using "DOMContentLoaded", you ensure that all relevant elements have been loaded regardless of how the script is added to the page, which is great!
Here it is also more preferable to use "DOMContentLoaded" over "load", since we only want to wait until the DOM has loaded; the loading of resources is irrelevant for our implementation.
getElementById()In this case, using getElementById() is more expressive than using querySelector() with an ID selector, because it conveys our intent more clearly: Getting an element by its ID. The approach of using querySelector() with an ID selector would be less clear at a glance.
Note that you can practically add IDs to every element, but reserving them for when it makes sense keeps your code complexity maintainable.
However, if your document structure allows for (subjectively) simple selectors to uniquely select elements, it may be preferable to use querySelector() instead of adding an ID solely to use getElementById().
letThe declarator var has been de facto deprecated in favour of let/const for various reasons, so usually the new declarators are preferred.
While let may be used in all places where const is used, its usage conveys a different intent:
const is used for a constant reference, whereas the reference of let may change during runtime.
Sidenote: Referenced values may still be mutated, including values referenced by const. Primitive values are immutable, but e.g. objects are mutable.
Regardless, learning JavaScript with let/const is usually easier and more preferable than learning it with var.
Useful comments are always appreciated. Since your function name is not expressive, your comment provides the missing description.
However, if your function name would be expressive enough (see section "Naming conventions" below), the comment would be obsolete.
type="button"By default, buttons have type="submit", which submits the form on click. But you want to use your button as an actual button.
This could be realized by calling event.preventDefault() in its listener, but using type="button" instead makes for semantic and more expressive HTML code and keeps your JS code simpler.
Unfortunately, it ambiguates whether the button-click would submit the form when only inspecting the JS code, but this is a trade-off for keeping the JS code simple(r).
Some elements (such as <fieldset>) are more rare on typical (commercial?) pages but more common on document-like pages. I personally enjoy seeing these uncommon elements being used.
Note that <fieldset> is usually used together with <legend> to provide a descriptive heading for it.
Your attempt of providing a label for the button is great!
A label's for attribute is used to reference another element by its ID. Unfortunately, your button doesn't have an ID, nor does the referenced ID "button" exist.
Note that a button's content may act as that button's label, so an additional label may actually not be necessary.
Your placeholder attribute currently only holds placeholder text, presumably because this is just a StackOverflow snippet. But providing instructions is great and improves the user experience.
addEventListener() over onevent attributes/properties.Nowadays web technologies (HTML, CSS and JS) allow for Separation of concerns: You can separate structural code (HTML), presentational code (CSS) and functional code (JS). Separating your code into these "concerns" allows for easier maintenance in the future.
That said, you are adding functionality to your page in two ways:
onclick.addEventListener().As mentioned before, functionality is more related to JS than HTML, so we should prefer addEventListener() to keep our concerns organized.
Sidenote: Adding the listener twice actually cancels its effect since it would be called twice per interaction, toggling back and forth between two states. Adding only one listener conveniently fixes this bug.
Using addEventListener() is also more preferred than assigning to the onclick property (or similar), because addEventListener() ...
window, document, etc.The HTMLElement.style property reflects that element's inline style declarations. You hide the element by setting style.display = "none", which is fine. But to unhide you assume that the previous value was block.
If the element's display value was not block, then your toggling behaviour may appear buggy. To fix this we should remove our assumption and just revert to the actual previous value.
We can revert to the previous value in multiple ways, but the easiest is to just remove our inline declaration:
const [revertingButton, overridingButton] = document.querySelectorAll("button");
revertingButton.addEventListener("click", evt => {
const button = evt.target.closest("button");
const div = button.nextElementSibling;
if (div.style.display === "none") {
// Remove inline declaration
div.style.display = "";
} else {
div.style.display = "none";
}
});
overridingButton.addEventListener("click", evt => {
const button = evt.target.closest("button");
const div = button.nextElementSibling;
if (div.style.display === "none") {
// Override style declaration with assumption
div.style.display = "block";
} else {
div.style.display = "none";
}
});div {display: flex} /* Notice this declaration! */
/* Ignore; for presentational purposes */
body {font-family: sans-serif}
div::before, div::after {
content: "";
width: 50px;
height: 50px;
display: block;
}
div::before {background-color: lightpink}
div::after {background-color: lightblue}
section {margin-block: .8rem}
header {
font-size: large;
font-weight: bold;
}<section>
<header>Removing inline declaration</header>
<button>Toggle visibility</button>
<div></div>
</section>
<section>
<header>Assuming previous value</header>
<button>Toggle visibility</button>
<div></div>
</section>Some of your names are either confusing or not meaningful (e.g. function name clickMe).
This can be fixed by following a naming convention. You can either decide on one by yourself, or choose (from) established naming conventions. Any naming convention is better than none. Here are some naming conventions:
By following the point "meaningful names" we may implement these changes:
clickMe to toggleInputVisibility.load to loadHandler, or use an anonymous function since the surrounding code (the context) provides enough meaning.It is good to practice implementing behaviours such as toggle buttons yourself, but for conciseness, uniformity and accessibility you should prefer the browser-provided features in any real-world projects. That said, let's focus on the accessibility aspect:
Controls (e.g. your toggling button) should indicate their state and what element they control. Referencing the controlled element is done with aria-controls.
Indicating the control's state can in our case be done with aria-checked since our button is a 2-state control, or by using an <input type="checkbox"> which natively indicates its own state.
Apart from indicating their state, checkboxes also indicate the correct role: Our button is of role="button" by default, but we effectively use it as a checkbox. Therefore this button's role should be checkbox.
Controls should also provide meaningful descriptions regarding their actions: The text "Click me!" is not descriptive. Instead, "Toggle input visiblity" (or just "Toggle visibility") would be better. If we used a checkbox instead of a button, its description should be provided in a label that references the checkbox with its for attribute.
Labels come with the additional benefit of increasing the interactable region for their referenced input element. For example, clicking a textfield's label focuses the textfield, or clicking a checkbox's label toggles the checkbox.
Also important to note is that "placeholder text is not a replacement for labels". When using input elements, always provide a label:
aria-labelledby, oraria-label, or<label>s and the for attribute.Nesting an input element inside a label element is technically enough according to the HTML specification, but "generally, explicit labels are better supported".
Here is an accessible example of using a button as a visibility toggle:
const button = document.querySelector("button");
button.addEventListener("click", () => {
// ariaChecked is a string, but we want to invert its boolean equivalent
button.ariaChecked = !(button.ariaChecked === "true");
const input = document.getElementById(button.getAttribute("aria-controls"));
input.style.display = button.ariaChecked === "true" ? "" : "none";
});/* Add visual indicator */
button::before {display: inline}
button[aria-checked=true]::before { content: "\2705 "}
button[aria-checked=false]::before { content: "\274E "}<div>
<button role="checkbox" aria-checked="true" aria-controls="button-input">Toggle visibility</button>
<input id="button-input">
</div>And here is an equivalent example but using a checkbox:
const checkbox = document.querySelector("input[type=checkbox]"); // or #checkbox
// Prefer to use "change" over "click" for semantics, but both work
checkbox.addEventListener("change", () => {
// Toggling checkedness is default behaviour
const input = document.getElementById(checkbox.getAttribute("aria-controls"));
input.style.display = checkbox.checked ? "" : "none";
});<div>
<label for="checkbox">
<input id="checkbox" type="checkbox" checked aria-controls="checkbox-input">Toggle visibility
</label>
<input id="checkbox-input">
</div>Things to notice:
.checked is boolean whereas .ariaChecked is a string.change), not to the cause of a state change (event type click) which may have been done manually.