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> -->

CodePudding user response：

Two things:

You don't need to add both the onclick attribute and an event listener. Just use .addEventListener, it is the preferred way.
Don't use tag selectors unless you really want to affect every <button> element. In your case, you should use an ID, such as "clickMeBtn".

document.addEventListener("DOMContentLoaded", load);

function load() {
  let button = document.querySelector("#clickMeBtn");
  button.addEventListener("click", clickMe);
}

// Click Function to change the display value of the input
function clickMe() {
  let popup = document.getElementById("popup");
  if (popup.style.display == "none") {
    popup.style.display = "block";
  } else {
    popup.style.display = "none";
  }
}

<form>
  <label for="button"></label>
  <fieldset>
    <ol>
      <li><button type="button" id="clickMeBtn">Click me!</button></li>
      <li><input type="text" id="popup" name="popup" placeholder="placeholder"></li>
    </ol>
  </fieldset>
</form>

CodePudding user response：

JS will execute code from top to bottom, in your code, you define function ClickMe() after your function load() so it can not call.

Just move it on.

There also other solution, that you dont need to add addEventListener with event click because in button tag already have default event click, just remove that event can solve this problem

CodePudding user response：

Best way to achieve this is to create a CSS class to hide and unhide the element.

document.querySelector("button").addEventListener("click", function (evt) {
  document.querySelector("input").classList.toggle("hidden");
});

.hidden{
display:none
}

<button>click</button>
<input type=text></input>

CodePudding user response：

You are adding the click listener twice, in HTML with onclick and in JavaScript with addEventListener(). Therefore it is called twice for each click.

Since your listener toggles between two states, calling it twice is effectively the same as calling it none at all. This is the behaviour your are observing.

Only assigning it once solves your issue. Prefer to keep addEventListener(); reasons are stated later on.

Since you mentioned that you "appreciate any feedback", I collected a few points that may be of interest:

TL;DR

Prefer addEventListener() over onevent attributes/properties.
Prefer reverting styles over overriding with assumed values.
Follow a naming convention, your own or an established one. This keeps names meaningful.
Prefer browser-provided features (checkbox) over custom implementations (custom toggle button).
Keep accessibility in mind.

Separation of concerns

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:

In HTML with onclick.
In JS with 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 onclick property (or similar), because addEventListener() ...

Allows adding multiple listeners.
Allows more configurations for the listener.
Exists on many different objects, e.g. elements, window, document, etc.

Inline styles

The HTMLElement.style property reflects the inline style declarations of that element. To hide your element, you set style.display = "none", which is fine. But to unhide you assume that the previous value was block.

If the element's display value is not block, then your toggling behaviour may appear buggy. To fix this we should remove our assumption and just revert to the previous value.

We can revert to the previous value in multiple ways, but the easiest is to just remove our inline declaration:

const [overridingButton, revertingButton] = document.querySelectorAll("button");

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";
  }
});
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";
  }
});

div {display: flex}

/* 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;
}

<p>The following DIVs are initially <code>display: flex</code>.</p>

<section>
  <header>With assumption</header>
  <button>Toggle visibility</button>
  <div></div>
</section>

<section>
  <header>With removing inline declaration</header>
  <button>Toggle visibility</button>
  <div></div>
</section>

Naming conventions

Some of your names are either confusing or not meaningful (function name clickMe).

This can be fixed by following a naming convention. You can either decide on one by yourself to keep your names standardized, or pick an established naming convention. Any naming convention is better than none. Here are some naming conventions:

Google's style guide: Simple and loose convention.
- No abbreviations.
- Meaningful over short names.
MDN's style guide: Stricter but still quite loose convention.
- No abbreviations.
- Short names.
- No articles or possessives.
- No type information in names (no "list", "array", "map") and no Hungarian Notation.

Following "meaningful names" we may implement the following changes:

Change function name clickMe to toggleInputVisibility.
Change function name load to loadHandler, or use an anonymous function since the context would provide enough meaning.

Accessibility

It is good practice to implement behaviours such as toggle buttons yourself, but for conciseness, uniformity and accessibility you should usually prefer the browser-provided features. 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 naturally of role: button, but we effectively use it as a checkbox. Therefore this button's role should be checkbox.

Labels

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:

With aria-labelledby, or
With aria-label, or
With <label>s and its 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".

Examples

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:

In the button-example we repurpose an element to behave like another. This makes the code less readable and more confusing. The HTML in the checkbox-example is therefore (subjectively) more understandable.
Toggling the checkedness comes built-in in the checkbox-example.
Using the checkedness of a checkbox is easier than using the checkedness of an ARIA-enhanced element, because .checked is boolean whereas .ariaChecked is a string.
(Subjective:) Semantically, we want to react to a state change (event type change). We do not want to react to the cause of the state change (event type click), where we potentially have to change the state manually.
The input fields in both examples are generic for simplicity's sake, therefore they do not have an associated label. As mentioned before, input elements should always be associated with a label in real-world scenarios.